-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
urls: introduce invenio_url_for-compatible links #609
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ | |
from copy import deepcopy | ||
|
||
from flask import current_app | ||
from invenio_base import invenio_url_for | ||
from invenio_records.dictutils import dict_lookup | ||
from uritemplate import URITemplate | ||
from werkzeug.datastructures import MultiDict | ||
|
@@ -95,7 +96,7 @@ def expand(self, identity, obj): | |
|
||
|
||
class Link: | ||
"""Utility class for keeping track of and resolve links.""" | ||
"""Deprecated utility class for keeping track of and resolve links.""" | ||
|
||
def __init__(self, uritemplate, when=None, vars=None): | ||
"""Constructor.""" | ||
|
@@ -125,6 +126,63 @@ def expand(self, obj, context): | |
return self._uritemplate.expand(**vars) | ||
|
||
|
||
class Link2: | ||
"""Encapsulation of the rendering of an endpoint URL. | ||
Is interface-compatible with Link for ease of initial adoption. | ||
""" | ||
|
||
def __init__(self, endpoint, when=None, vars=None, params=None): | ||
"""Constructor. | ||
:param endpoint: str. endpoint of the URL | ||
:param when: fn(obj, dict) -> bool, when the URL should be rendered | ||
:param vars: fn(ob, dict), mutate dict in preparation for expansion | ||
:param params: list, parameters (excluding querystrings) used for expansion | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Otherwise a bunch of other stuff gets shoved into the built url, see explainer below. If anything, we have less to specify when defining Link2s than Links (no |
||
""" | ||
self._endpoint = endpoint | ||
self._when_func = when | ||
self._vars_func = vars | ||
self._params = params or [] | ||
|
||
def should_render(self, obj, context): | ||
"""Determine if the link should be rendered.""" | ||
if self._when_func: | ||
return bool(self._when_func(obj, context)) | ||
return True | ||
|
||
@staticmethod | ||
def vars(obj, vars): | ||
"""Dynamically update vars used to expand the link. | ||
Subclasses should overwrite this method. | ||
""" | ||
pass | ||
|
||
def expand(self, obj, context): | ||
"""Expand the endpoint. | ||
Note: "args" key in generated values for expansion has special meaning. | ||
It is used for querystring parameters. | ||
""" | ||
vars = {} | ||
vars.update(deepcopy(context)) | ||
self.vars(obj, vars) | ||
if self._vars_func: | ||
self._vars_func(obj, vars) | ||
|
||
# Construct final values dict. | ||
# Because invenio_url_for renders on the URL all arguments given to it, | ||
# filtering for expandable ones must be done. | ||
values = {k: v for k, v in vars.items() if k in self._params} | ||
# The "args" key in the final values dict is where | ||
# querystrings are passed through. | ||
# Assumes no clash between URL params and querystrings | ||
values.update(vars.get("args", {})) | ||
values = dict(sorted(values.items())) # keep sorted interface | ||
return invenio_url_for(self._endpoint, **values) | ||
|
||
|
||
class ConditionalLink: | ||
"""Conditional link.""" | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,13 +10,11 @@ | |
"""Record Service API.""" | ||
|
||
from ..base import ServiceConfig | ||
from ..records.links import RecordLink | ||
from .components import ( | ||
FileContentComponent, | ||
FileMetadataComponent, | ||
FileProcessorComponent, | ||
) | ||
from .links import FileLink | ||
from .processors import ImageMetadataExtractor | ||
from .results import FileItem, FileList | ||
from .schema import FileSchema | ||
|
@@ -41,14 +39,9 @@ class FileServiceConfig(ServiceConfig): | |
|
||
max_files_count = 100 | ||
|
||
file_links_list = { | ||
"self": RecordLink("{+api}/records/{id}/files"), | ||
} | ||
|
||
file_links_item = { | ||
"self": FileLink("{+api}/records/{id}/files/{+key}"), | ||
"content": FileLink("{+api}/records/{id}/files/{+key}/content"), | ||
} | ||
# Inheriting resource config should define these | ||
file_links_list = {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
file_links_item = {} | ||
|
||
components = [ | ||
FileMetadataComponent, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,11 +40,21 @@ def file_result_list(self, *args, **kwargs): | |
|
||
def file_links_list_tpl(self, id_): | ||
"""Return a link template for list results.""" | ||
return LinksTemplate(self.config.file_links_list, context={"id": id_}) | ||
return LinksTemplate( | ||
# Until all modules have transitioned to using invenio_url_for, | ||
# we have to keep `id` in context for URL expansion | ||
self.config.file_links_list, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In following major bump, we can remove. |
||
context={"id": id_, "pid_value": id_}, | ||
) | ||
|
||
def file_links_item_tpl(self, id_): | ||
"""Return a link template for item results.""" | ||
return LinksTemplate(self.config.file_links_item, context={"id": id_}) | ||
return LinksTemplate( | ||
# Until all modules have transitioned to using invenio_url_for, | ||
# we have to keep `id` in context for URL expansion | ||
self.config.file_links_item, | ||
context={"id": id_, "pid_value": id_}, | ||
) | ||
|
||
def check_permission(self, identity, action_name, **kwargs): | ||
"""Check a permission against the identity.""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
# -*- coding: utf-8 -*- | ||
# | ||
# Copyright (C) 2025 Northwestern University. | ||
# | ||
# Invenio-RDM-Records is free software; you can redistribute it and/or modify | ||
# it under the terms of the MIT License; see LICENSE file for more details. | ||
|
||
"""Tests.""" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This and most following changes is as inveniosoftware/invenio-rdm-records#1954 (for pytest v8 and better entry_point module discovery to be honest) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,9 +16,9 @@ | |
import pytest | ||
from flask_principal import Identity, Need, UserNeed | ||
from invenio_app.factory import create_api as _create_api | ||
from mock_module.config import MockFileServiceConfig, ServiceConfig | ||
|
||
from invenio_records_resources.services import FileService, RecordService | ||
from tests.mock_module.config import FileServiceConfig, ServiceConfig | ||
|
||
pytest_plugins = ("celery.contrib.pytest",) | ||
|
||
|
@@ -48,6 +48,8 @@ def app_config(app_config): | |
"invenio_jsonschemas.proxies.current_refresolver_store" | ||
) | ||
|
||
app_config["THEME_FRONTPAGE"] = False | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needed because invenio-theme uses subscription operator to check its value see https://github.com/inveniosoftware/invenio-theme/blob/master/invenio_theme/views.py#L24 . I also made a PR to fix that: inveniosoftware/invenio-theme#409 . Discussion: we could also fix that (use |
||
|
||
return app_config | ||
|
||
|
||
|
@@ -56,13 +58,18 @@ def extra_entry_points(): | |
"""Extra entry points to load the mock_module features.""" | ||
return { | ||
"invenio_db.model": [ | ||
"mock_module = mock_module.models", | ||
"mock_module = tests.mock_module.models", | ||
], | ||
"invenio_jsonschemas.schemas": [ | ||
"mock_module = mock_module.jsonschemas", | ||
"mock_module = tests.mock_module.jsonschemas", | ||
], | ||
"invenio_search.mappings": [ | ||
"records = mock_module.mappings", | ||
"records = tests.mock_module.mappings", | ||
], | ||
"invenio_base.api_blueprints": [ | ||
"mock_module_mocks = tests.mock_module:create_mocks_bp", | ||
# still present even though above doesn't support files | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. which is per the original tests |
||
"mock_module_mocks_files = tests.mock_module:create_mocks_files_bp", | ||
], | ||
} | ||
|
||
|
@@ -76,7 +83,7 @@ def create_app(instance_path, entry_points): | |
@pytest.fixture(scope="module") | ||
def file_service(): | ||
"""File service shared fixture.""" | ||
return FileService(MockFileServiceConfig) | ||
return FileService(FileServiceConfig) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It wasn't |
||
|
||
|
||
@pytest.fixture(scope="module") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order for the release of this package to not take down other packages using
Link
, a different name is used (different class rather than editing same class). The nameLink2
(and so on for other constructs) was the simplest.