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

Why Api is already rendering to JSON/HTML #1802

Open
vvmruder opened this issue Sep 3, 2024 · 6 comments
Open

Why Api is already rendering to JSON/HTML #1802

vvmruder opened this issue Sep 3, 2024 · 6 comments
Labels
enhancement New feature or request

Comments

@vvmruder
Copy link
Contributor

vvmruder commented Sep 3, 2024

Is your feature request related to a problem? Please describe.

It is not a problem but an architectural question. And before I put too much effort in this I wanted to ask here. I work on a project which like to integrate pygeoapi for at least the OAPIF part. Reviewing the current code, I wonder about the level where actual rendering of results takes place. We can take get_collection_queryables as an example. But it is valid for all API endpoints.

In my opinion, rendering to json/html already here is a bit too early since it blocks an integrating app from hooking into the process to inject specific things. Maybe we want to add extra links, which are app specific. Or apply additional filters on the result set before it is delivered as response. Currently one option I do have, is to subclass the API , overwrite the desired function (where I have probably 99% copied from the original one). The second option is to load responded JSON to dict again in the calling method of my project to manipulate it and then redump it to json again. Both solutions are not really sufficient. The first not because it will introduce undeniable problems when it comes to upgrading pygoapi over time. The second is a perfomance killer and especially for the OAPIF not a real option.

In my opinion rendering should be left to the actual framework (django, flask, etc) completely. Instead of solving rendering in the API of pygeoapi we should invest a bit to correctly type the interface of the responses (with eg. dataclasses).

I kindly offer my support to help with that if this idea is accepted.

Describe the solution you'd like
It can be solved by completely getting rid of any Jinja render or JSON dump logic at the API class levels. Instead this should be a task for the framework actually producing it. I can imagine that we still could offer flask and django default apps as it is done today. But with a bit more logic in these dedicated methods (e.g. django collection_queryables). The rendering should teka place in these methods.

Describe alternatives you've considered
Alternatives are the 2 approaches I described in the problem description.

Additional context
no addional context

@vvmruder vvmruder added the enhancement New feature or request label Sep 3, 2024
@vvmruder vvmruder changed the title Why Api is already rendering to JSON Why Api is already rendering to JSON/HTML Sep 3, 2024
@m-kuhn
Copy link

m-kuhn commented Oct 4, 2024

@tomkralidis @webb-ben @kalxas any opinions on this proposal of separating API logic from rendering?

@totycro
Copy link
Contributor

totycro commented Oct 16, 2024

I wasn't present when the initial implementation was started, but from what i understood from working with pygeoapi, the intention was to have as little web framework specific code as possible. I agree that it's' not a common architecture that the API implementation already renders the data to json, but it does make the flask/django/starlette adapter straightforward. Therefore updates of those web frameworks are usually trivial, however at the cost that the API implementations has to do this low level work.

This makes pygeoapi hard to embed as a library as described above, but i guess the usual way of embedding it would be to include the web framework specific code as views or as an app.

If we were to change this, it would touch a lot of code and it might not be easy to get all cases right. If we want pygeoapi to be callable by other python code and not only by embedding the API views, i think it's necessary though.

@webb-ben
Copy link
Member

webb-ben commented Dec 6, 2024

Hi @vvmruder, I would be interested in hearing more about your use case. I will not answer on the rendering of JSON inside API as it also predates me, but will try to provide some potential solutions that might be compatible with your requests while involving minimal changes to pygeoapi core.

For any changes you want to make to the Features, I would wonder if a similar approach to what I implemented for our SPARQL provider might work? Essentially a wrapper provider that invokes the actual pygeoapi provider, and then applies the necessary changes to the Feature. In this way, we are intercepting the content before it ever reaches the API. In our case, we are enriching the properties block with information fetched from a remote SPARQL provider, but I am sure it would be possible to have a provider that implements custom filtering of Features.

As for the desire to include additional links, I am going to assume it is some link relation that you want to dynamically generate and thus cannot provide in the pygeoapi configuration. I wonder if my above solution with a slight modification to get_collection_items to respect links provided from the pygeoapi provider like we do in get_collection_item

@tomkralidis
Copy link
Member

tomkralidis commented Dec 8, 2024

@vvmruder @m-kuhn sorry for the delay. Keeping the web "frontends" (Django, Flask, Starlette) minimal is a design decision in pygeoapi so as to keep them as light as possible. This means anyone adding new APIs to the core would have minimal work inside the top level request/response workflow.

@francbartoli
Copy link
Contributor

@vvmruder @m-kuhn is your intention to only inject the rendering of JSON, HTML or whatever into the API logic or instead to decouple them completely changing the signature of the current core API functions? Can you provide a very bare minimal example?

@vvmruder
Copy link
Contributor Author

Hi everone. Thank you for this feedback. I'm happy to provide an exemplaric PR in the next days.

Regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants