setup: add api entry points

[jrcastro2]

No description provided.

[egabancho]

Have you checked if this affects the API error handlers? i.e. https://github.com/inveniosoftware/invenio-theme/blob/master/invenio_theme/ext.py#L84 vs https://github.com/inveniosoftware/invenio-rest/blob/master/invenio_rest/ext.py#L59

[jrcastro2]

Have you checked if this affects the API error handlers? i.e. https://github.com/inveniosoftware/invenio-theme/blob/master/invenio_theme/ext.py#L84 vs https://github.com/inveniosoftware/invenio-rest/blob/master/invenio_rest/ext.py#L59

Thanks for raising the concern. I just checked, and there shouldn’t be any reason for this change to affect the API error handlers. The two approaches you linked (app.errorhandler(...) and app.register_error_handler(...)) are functionally equivalent in Flask for registering error handlers (see here). In addition, there’s test coverage that verifies the error handlers’ behavior, and all tests are passing without issues.

[slint]

I agree with @egabancho, we're registering conflicting error handlers for the same response codes, and just because the extension initialization order is random we might getting false positives on tests (i.e. if InvenioTheme runs first, and afterwards InvenioREST).

The approach we usually take in these cases is to have two separate extensions for a UI and REST API version, so the existing InvenioTheme, and InvenioThemeREST which skips parts of the initialization that we don't want for the REST API.

[fenekku]

What @slint said :) .

For more context since I delved into this when working on invenio_url_for:

InvenioTheme is only loaded in UI app - this makes sense because REST app doesn't need any UI elements and doesn't want error handlers that would generate HTML pages rather than JSON with error message i.e., like Alex said the double registration would create inconsistencies.

But for generating urls via invenio_url_for, invenio_theme.views:create_blueprint is registered on a temporary app because invenio_base.blueprints is looked up. So if THEME_FRONTPAGE is not loaded in the config, the [] access in create_blueprint will raise a KeyError. That's why it's added in tests so that the API application gets it (e.g., https://github.com/inveniosoftware/invenio-rdm-records/blob/master/tests/conftest.py#L394 ).

But then, how is it available in a live application's API container? (did I ask myself with alarm) It's actually because invenio-app-rdm has this entrypoint: https://github.com/inveniosoftware/invenio-app-rdm/blob/master/setup.cfg#L101 which makes its definition of THEME_FRONTPAGE in config.py be loaded up, thanks to invenio-config .

Bottom line: we shouldn't add invenio-theme to api_apps entrypoint and the setting should be injected in the context you need instead.

[slint]

Now that I understand that this PR is actually about THEME_FRONTPAGE missing in some contexts and after seeing it being slowly sprinkled throughout conftest.pys, I have a feeling that this expectation should not be enforced.

• For regular modules, we should only need to set THEME_FRONTPAGE (or any view/blueprint-related config) in conftest.py if they generate URLs for these views. E.g. invenio-communities never has to deal with the frontpage view, so it wouldn't care if THEME_FRONTPAGE was set at all (since it's not going to call invenio_url_for for it).
• App modules (invenio-app-rdm and invenio-app-ils) and instances that rely on these features make use of the invenio-config entrypoint to declare this early in the global config-loading process to solve this.

This is my logical expectation for how config should work. If we have mandatory []-access for config variables in the code, we should only be doing this if we know for sure that this code path will have this config set. If not, then we have to be defensive and app.config.get("<MY_CONFIG>") it.

[fenekku]

To further clarify: even if invenio_url_for is not called, the url Map is created at App creation such that if invenio-theme for instance is a dependency, its blueprint endpoint will be registered by the wider invenio_url_for mechanism and therefore THEME_FRONTPAGE would need to be set. In other words, if a module's blueprint registration requires a setting, that setting needs to be at minimum present in both API and UI apps.

For the [] access, that was the context in which I was wondering about converting it to .get() some time ago. I think we settled on saying that [] was to force some settings to be set and fail early. Is that still the case? (it's fair either way). I thought there was also a proposal at some point to have a dedicated settings validation step, but I can't find the link so maybe I am imagining it 😅 .

Have we helped the original PR or gone way off tangent 😆 @jrcastro2 ? I think the action item is still to not add this to invenio_base.api_apps, and add "THEME_FRONTPAGE" in the context you need, but maybe there is something else too?

[jrcastro2]

Thanks a lot for all the clarifications, I see now I missunderstood the original comment 😅

IMHO I agree with the defensive approach of using .get(). Since this value isn't guaranteed to be present (unless we add it to api_apps), the alternarive would be to patch it across multiple modules just to avoid issues in contexts like Alembic upgrades.

Let me know if you have other opinons on this, and thanks a lot for the comments!

[slint]

If it solves the issue, I would recommend we try to switch to .get() for fetching the THEME_FRONTPAGE config (and default to whatever value is in invenio_theme/config.py).

@jrcastro2 (or @palkerecsenyi) if you can also add to the PR description some more info or link to what the original issue was for having to add this, it would help. In any case, whatever the decision, we should have a good commit message, since it'll be a small change with a bigger impact to the whole codebase.

[fenekku]

Thanks for bearing with that!