urls: introduce invenio_url_for-compatible links #609
Conversation
fenekku
force-pushed
the
integrate_invenio_url_for_on_top_master
branch
from
59134ea to
dba04a3
Compare
| @@ -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 | |||
There was a problem hiding this comment.
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.
| :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.
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}).
| } | ||
| # Inheriting resource config should define these | ||
| # TODO: Edit rdm-records such that it doesn't rely on this first | ||
| file_links_list = {} |
There was a problem hiding this comment.
| 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.
In following major bump, we can remove.
| # 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.
This and most following changes is as inveniosoftware/invenio-rdm-records#1954 (for pytest v8 and better entry_point module discovery to be honest)
| service_for_records = RecordService(ServiceConfig) | ||
| service_for_records_w_files = RecordService(ServiceWithFilesConfig) | ||
| service_for_files = FileService(FileServiceConfig) |
There was a problem hiding this comment.
Had to be done because of pre-existing tests.
| "self": RecordLink("{+api}/mocks/{id}"), | ||
| "files": RecordLink("{+api}/mocks/{id}/files"), | ||
| "files-archive": RecordLink("{+api}/mocks/{id}/files-archive"), | ||
| "self": RecordLink2("mocks.read", params=["pid_value"]), |
There was a problem hiding this comment.
The new way to define links serialization in action.
| def base_app(base_app, record_resource): | ||
| """Application factory fixture.""" | ||
| base_app.register_blueprint(record_resource.as_blueprint()) | ||
| yield base_app |
There was a problem hiding this comment.
By relying on entry points to load blueprints, this kind of thing is not needed anymore.
| @@ -279,8 +279,7 @@ def test_links_keep_facets(client, headers, three_indexed_records): | |||
| response_links = response.json["links"] | |||
| expected_links = { | |||
| "self": ( | |||
| "https://127.0.0.1:5000/api/mocks?" | |||
| "page=1&size=25&sort=newest&type=A%2A%2AB" | |||
| "https://127.0.0.1:5000/api/mocks?page=1&size=25&sort=newest&type=A**B" | |||
There was a problem hiding this comment.
invenio_url_for (really werkzeug url building) doesn't % encode (it's fine).
| ids=[item_one.id, item_two.id], | ||
| # NOTE: "id" and "pid" are needed to produce the links in the result | ||
| # Previous link generation would silently produce incorrect links | ||
| fields=["id", "pid", "metadata.type.type"], |
There was a problem hiding this comment.
Discussion: yeah this is good and bad. Essentially the new way of generating links informs us of cases where urls were malformed... by throwing a BuildError. I don't think "id" and "pid" are universal though... If they are, we could add them to https://github.com/inveniosoftware/invenio-records-resources/blob/master/invenio_records_resources/services/records/service.py#L440 or make those default dumper fields configurable in RecordServiceConfig.
Not required for initial invenio-rdm-records PR - we can introduce the constructs from this PR into it later. But inveniosoftware/invenio-rdm-records#1962 is required before this is released.
The high number of changed files is just because of the
tests/__init__.pypattern (see inveniosoftware/invenio-rdm-records#1954).A guided commentary tour is provided.