Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion invenio_records_resources/services/base/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@
"""Base Service API."""

from .config import ServiceConfig
from .links import ConditionalLink, Link, LinksTemplate, NestedLinks
from .links import ConditionalLink, Link, Link2, LinksTemplate, NestedLinks
Copy link
Contributor Author

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 name Link2 (and so on for other constructs) was the simplest.

from .results import ServiceItemResult, ServiceListResult
from .service import Service

__all__ = (
"ConditionalLink",
"Link",
"Link2",
"LinksTemplate",
"Service",
"ServiceConfig",
Expand Down
60 changes: 59 additions & 1 deletion invenio_records_resources/services/base/links.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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."""
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 {api/ui}, no {args}).

"""
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."""

Expand Down
13 changes: 3 additions & 10 deletions invenio_records_resources/services/files/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file_links_item = {}

components = [
FileMetadataComponent,
Expand Down
15 changes: 12 additions & 3 deletions invenio_records_resources/services/files/links.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
# -*- coding: utf-8 -*-
#
# Copyright (C) 2020-2021 CERN.
# Copyright (C) 2020-2021 Northwestern University.
# Copyright (C) 2020-2025 Northwestern University.
#
# Flask-Resources is free software; you can redistribute it and/or modify it
# under the terms of the MIT License; see LICENSE file for more details.

"""Utility for rendering URI template links."""

from ..base import Link
from ..base import Link, Link2


class FileLink(Link):
"""Short cut for writing record links."""
"""Deprecated shortcut for writing file links."""

@staticmethod
def vars(file_record, vars):
Expand All @@ -22,3 +22,12 @@ def vars(file_record, vars):
"key": file_record.key,
}
)


class FileLink2(Link2):
"""Rendering of a file link with specific vars expansion."""

@staticmethod
def vars(file_record, vars):
"""Variables for the endpoint expansion."""
vars.update({"key": file_record.key})
14 changes: 12 additions & 2 deletions invenio_records_resources/services/files/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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."""
Expand Down
12 changes: 5 additions & 7 deletions invenio_records_resources/services/records/config.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# -*- coding: utf-8 -*-
#
# Copyright (C) 2020-2024 CERN.
# Copyright (C) 2020 Northwestern University.
# Copyright (C) 2020-2025 Northwestern University.
# Copyright (C) 2023 Graz University of Technology.
#
# Invenio-Records-Resources is free software; you can redistribute it and/or
Expand All @@ -18,7 +18,7 @@
from ...records import Record
from ..base import ServiceConfig
from .components import MetadataComponent
from .links import RecordLink, pagination_links
from .links import RecordLink, RecordLink2, pagination_links
from .params import FacetsParam, PaginationParam, QueryParser, QueryStrParam, SortParam
from .results import RecordBulkItem, RecordBulkList, RecordItem, RecordList

Expand Down Expand Up @@ -75,11 +75,9 @@ class RecordServiceConfig(ServiceConfig):
# Service schema
schema = None # Needs to be defined on concrete record service config

links_item = {
"self": RecordLink("{+api}/records/{id}"),
}

links_search = pagination_links("{+api}/records{?args*}")
# Definition of those is left up to implementations
links_item = {}
links_search = {}

# Service components
components = [
Expand Down
43 changes: 39 additions & 4 deletions invenio_records_resources/services/records/links.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
# -*- coding: utf-8 -*-
#
# Copyright (C) 2020-2021 CERN.
# Copyright (C) 2020-2021 Northwestern University.
# Copyright (C) 2020-2025 Northwestern University.
#
# Flask-Resources is free software; you can redistribute it and/or modify it
# under the terms of the MIT License; see LICENSE file for more details.

"""Utility for rendering URI template links."""

from ..base import Link
from ..base import Link, Link2


class RecordLink(Link):
"""Short cut for writing record links."""
"""Deprecated shortcut for writing record links."""

@staticmethod
def vars(record, vars):
Expand All @@ -23,8 +23,20 @@ def vars(record, vars):
vars.update({"id": record.pid.pid_value})


class RecordLink2(Link2):
"""Rendering of a record link with specific vars expansion."""

@staticmethod
def vars(record, vars):
"""Variables for the endpoint expansion."""
# Some records don't have record.pid.pid_value yet (e.g. drafts)
pid_value = getattr(record.pid, "pid_value", None)
if pid_value:
vars.update({"pid_value": record.pid.pid_value})


def pagination_links(tpl):
"""Create pagination links (prev/selv/next) from the same template."""
"""Create pagination links (prev/self/next) from the same template."""
return {
"prev": Link(
tpl,
Expand All @@ -42,3 +54,26 @@ def pagination_links(tpl):
),
),
}


def pagination_links2(endpoint, params=None):
"""Create pagination links (prev/self/next) from the same template."""
return {
"prev": Link2(
endpoint,
when=lambda pagination, ctx: pagination.has_prev,
vars=lambda pagination, vars: vars["args"].update( #
{"page": pagination.prev_page.page}
),
params=params,
),
"self": Link2(endpoint, params=params),
"next": Link2(
endpoint,
when=lambda pagination, ctx: pagination.has_next,
vars=lambda pagination, vars: vars["args"].update( # ["args"]
{"page": pagination.next_page.page}
),
params=params,
),
}
2 changes: 1 addition & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ install_requires =
[options.extras_require]
tests =
pytest-black-ng>=0.4.0
invenio-app>=2.0.0,<3.0.0
invenio-app>=2.1.0,<3.0.0
invenio-db[postgresql,mysql,versioning]>=2.0.0,<3.0.0
pytest-invenio>=3.0.0,<4.0.0
pytest-mock>=1.6.0
Expand Down
8 changes: 8 additions & 0 deletions tests/__init__.py
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."""
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)

17 changes: 12 additions & 5 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",)

Expand Down Expand Up @@ -48,6 +48,8 @@ def app_config(app_config):
"invenio_jsonschemas.proxies.current_refresolver_store"
)

app_config["THEME_FRONTPAGE"] = False
Copy link
Contributor Author

@fenekku fenekku Feb 20, 2025

Choose a reason for hiding this comment

The 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 get) and move the app.register_error_handler out of there in invenio-theme since they don't really have to do with that blueprint since they are app wide (could be registered in the app loading part).


return app_config


Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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",
],
}

Expand All @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't MockServiceConfig so aligned the name of both.



@pytest.fixture(scope="module")
Expand Down
7 changes: 3 additions & 4 deletions tests/factories/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
"""Factories test configuration."""

import pytest
from flask_principal import Identity, Need, UserNeed
from invenio_app.factory import create_api as _create_api


Expand All @@ -21,13 +20,13 @@ def extra_entry_points():
return {
# to be verified if needed, since the models are dynamically created
"invenio_db.model": [
"mock_module_factory = mock_module_factory.grant",
"mock_module_factory = tests.mock_module_factory.grant",
],
"invenio_jsonschemas.schemas": [
"mock_module_factory = mock_module_factory.jsonschemas",
"mock_module_factory = tests.mock_module_factory.jsonschemas",
],
"invenio_search.mappings": [
"grants = mock_module_factory.mappings",
"grants = tests.mock_module_factory.mappings",
],
}

Expand Down
Loading
Loading