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

Please allow to pass page and size directly to the query instead of having to instantiate a QueryParameter object #185

Open
b1rger opened this issue Jan 9, 2025 · 20 comments

Comments

@b1rger
Copy link
Collaborator

b1rger commented Jan 9, 2025

No description provided.

@lu-pl
Copy link
Contributor

lu-pl commented Jan 9, 2025

Passing atomic parameters directly to SPARQLModelAdapter.query has been discontinued in favor of Query Parameter Models.

For FastAPI apps this is best practice, but the solution is still compatible with other backend frameworks. Alternative frameworks will need to instantiate a QueryParameters object manually, which might be a minor but imo perfectly bearable inconvenience.

Apart from being able to exert more fine-grained control over query parameters and parameter validation, using a Query Parameter Model also results in better OpenAPI documentation.

So I am much in favor of sticking with Query Parameter Models.

Also there is an interesting metaclass hack that is only possible with a Query Parameter Model. This is not a strong argument of course, but shows that using a model for parameters potentially opens many interesting possibilities.

@b1rger
Copy link
Collaborator Author

b1rger commented Jan 9, 2025

For FastAPI apps this is best practice, but the solution is still compatible with other backend frameworks. Alternative frameworks will need to instantiate a QueryParameters object manually, which might be a minor but imo perfectly bearable inconvenience.

This means I have to either instantiate exactly rdfproxy.utils.models.QueryParameters or if I use my own QueryParameters model (for examply if i want to add additional paramters, like a search query) I have to be use the same attribute names as rdfproxy.utils.models.QueryParameters, otherwise there is an attribute error.

I tried using rdfproxy.utils.models.QueryParameters together with a CustomQueryParameters, but apparently fastapi isn't that clever.

@b1rger
Copy link
Collaborator Author

b1rger commented Jan 9, 2025

I've decided to let my CustomQueryParameters inherit from rdfproxy.utils.models.QueryParameters. But the expected shape of the query_parameters should be documented somewhere

@lu-pl
Copy link
Contributor

lu-pl commented Jan 9, 2025

Hm, one of the ideas of using Query Parameters is that implementers should control QueryParameters entirely, I am not sure if library users should attempt to implement filtering themselves.

@lu-pl
Copy link
Contributor

lu-pl commented Jan 9, 2025

I've decided to let my CustomQueryParameters inherit from rdfproxy.utils.models.QueryParameters. But the expected shape of the query_parameters should be documented somewhere

Sorry I think I don't get it. The expected interface for code handling QueryParameter objects obviously is that of QueryParameters (as indicated by the respective type annotation).

@b1rger
Copy link
Collaborator Author

b1rger commented Jan 9, 2025

Hm, one of the ideas of using Query Parameters is that implementers should control QueryParameters entirely, I am not sure if library users should attempt to implement filtering themselves.

I'm not sure who are "implementers" and who are "users", but filtering is one of our main usecases

@lu-pl
Copy link
Contributor

lu-pl commented Jan 9, 2025

Hm, one of the ideas of using Query Parameters is that implementers should control QueryParameters entirely, I am not sure if library users should attempt to implement filtering themselves.

I'm not sure who are "implementers" and who are "users", but filtering is one of our main usecases

I am aware that filtering is a very basic and very important feature, nonetheless it currently is not yet implemented.

Of course you can implement a filtering solution yourself. but available filter options won't be visible in the OpenAPI docs and care must be taken to not interfere with RDFProxy query modification.

In the case of filtering this should be doable, but it requires custom query construction within the route; ultimately, RDFProxy aims to provide a neat and standardized interface for filtering that is entirely controlled by the library.

@b1rger
Copy link
Collaborator Author

b1rger commented Jan 9, 2025

I've decided to let my CustomQueryParameters inherit from rdfproxy.utils.models.QueryParameters. But the expected shape of the query_parameters should be documented somewhere

Sorry I think I don't get it. The expected interface for code handling QueryParameter objects obviously is that of QueryParameters (as indicated by the respective type annotation).

no, the type annotation does not define the interface. it defines the type. so the type has to be exactly rdfproxy.utils.models.QueryParameter? We now have our own CustomQueryParameter which mimics the interface from rdfproxy.utils.models.QueryParameter (but I guess any hip typing IDE would panic and run away in horror, because we do actually pass an object that is NOT rdfproxy.utils.models.QueryParameter ) - is this a supported usecase?

@lu-pl
Copy link
Contributor

lu-pl commented Jan 9, 2025

I've decided to let my CustomQueryParameters inherit from rdfproxy.utils.models.QueryParameters. But the expected shape of the query_parameters should be documented somewhere

Sorry I think I don't get it. The expected interface for code handling QueryParameter objects obviously is that of QueryParameters (as indicated by the respective type annotation).

no, the type annotation does not define the interface. it defines the type. so the type has to be exactly rdfproxy.utils.models.QueryParameter? We now have our own CustomQueryParameter which mimics the interface from rdfproxy.utils.models.QueryParameter (but I guess any hip typing IDE would panic and run away in horror, because we do actually pass an object that is NOT rdfproxy.utils.models.QueryParameter ) - is this a supported usecase?

I wasn't aware that Emacs with an LSP backend was hip, but if typing is hip then I guess I'm a hipster! :D

A type generally denotes an upper bound of a type, so even the hippest of IDEs wouldn't panic if you passed a QueryParameters subclass.

@b1rger
Copy link
Collaborator Author

b1rger commented Jan 9, 2025

A type generally denotes an upper bound of a type

oh, wow. thats ... interesting

@b1rger
Copy link
Collaborator Author

b1rger commented Jan 9, 2025

In the case of filtering this should be doable, but it requires custom query construction within the route; ultimately, RDFProxy aims to provide a neat and standardized interface for filtering that is entirely controlled by the library.

oke, back to this statement: this should be discussed with the users. as the discussion above shows, rdfproxy already makes design decisions that force us into doing it the rdproxy way. it would be great if the filtering would keep working as it is now, and would not be "entirely controlled by the library".

@lu-pl
Copy link
Contributor

lu-pl commented Jan 13, 2025

Just a quick follow up:

Users can define whatever route parameters they want, e.g.:

@app.rout("/")
def some_route(page: int = 1, size: int = 100, whatever: Any | None = None)
    something = do_whatever(whatever)
    query_parameteres = QueryParameters(page=page, size=size)

    return adapter.query(query_parameters=query_parameteres)

The interface of SPARQLModelAdapter.query requires a QueryParameters object to be passed. And that's it. No need to subclass QueryParameters. QueryParameters is just the interface upon which rdfproxy code relies, so if you subclass it, rdfproxy of course won't access and processs custom slots, the only thing that happens in that case is that FastAPI will be able to generate enhanced OpenAPI docs for your custom fields.

@lu-pl
Copy link
Contributor

lu-pl commented Jan 13, 2025

QueryParameters defines an interface for functionality currently supported by RDFProxy. Once filtering and ordering will be implemented, those options will show up in the standard QueryParameters model.

So, I strongly feel that there should be an RDFProxy-way of doing things. But if you want to do your own thing or implement functionality currently not (yet!) supported, this is of course possible.

@kevinstadler
Copy link

In the case of filtering this should be doable, but it requires custom query construction within the route; ultimately, RDFProxy aims to provide a neat and standardized interface for filtering that is entirely controlled by the library.

oke, back to this statement: this should be discussed with the users. as the discussion above shows, rdfproxy already makes design decisions that force us into doing it the rdproxy way. it would be great if the filtering would keep working as it is now, and would not be "entirely controlled by the library".

What do you mean by filtering "working as it is now"? If you mean WHERE clauses in the query that you pass to the adapter: these clauses (which refer to SPARQL-binding names) will not be removed, but rdfproxy might dynamically add additional clauses based on a future filters parameter which, crucially, will refer to the output model field names (which are what the API consumer sees), and which rdfproxy will translate to SPARQL binding names in the background. Nothing is stopping you from doing SPARQL binding name-based query manipulation before you pass your query string to the adapter, and nothing will in the future. You will just find yourself rewriting 'filtering' (or 'search queries' or whatever) query manipulation for every route, when the plan is that rdfproxy will be doing this thing for you dynamically.

This means I have to either instantiate exactly rdfproxy.utils.models.QueryParameters or if I use my own QueryParameters model (for examply if i want to add additional paramters, like a search query)

This doesn't really make sense -- the QueryParameters config object is passed to adapter.query() which is an internal method, why would a library user want to add additional parameters to it when they're not controlling what happens inside with it inside the adapter?

@b1rger
Copy link
Collaborator Author

b1rger commented Jan 13, 2025

I'm closing this. It is clear that rdproxy does not suit our usecase in the long run and we'll find another solution

@b1rger b1rger closed this as completed Jan 13, 2025
@lu-pl lu-pl reopened this Jan 13, 2025
@lu-pl
Copy link
Contributor

lu-pl commented Jan 13, 2025

I'm closing this. It is clear that rdproxy does not suit our usecase in the long run and we'll find another solution

It would be good if you provided feedback and if you would explain, why exactly RDFProxy does not meet your usecase.

RDFProxy simply does not support filtering YET. You can inject FILTER clauses yourself and make it work. But once RDFProxy filtering is implemented, it will allow to filter by model fields which will also be visible in the OpenAPI docs. RDFPRoxy aims to provide a high level framework, so that users don't need to mess with query construction if they don't want to.

@lu-pl
Copy link
Contributor

lu-pl commented Jan 13, 2025

This doesn't really make sense -- the QueryParameters config object is passed to adapter.query() which is an internal method, why would a library user want to add additional parameters to it when they're not controlling what happens inside with it inside the adapter?

Yes. RDFProxy expects a QueryParameters object, the route parameters are a different matter, as mentioned, you can define whatever route parameters you want. But the interface of SPARQLModelAdapter.query expects a QueryParameters object.

@lu-pl
Copy link
Contributor

lu-pl commented Jan 13, 2025

This doesn't really make sense -- the QueryParameters config object is passed to adapter.query() which is an internal method, why would a library user want to add additional parameters to it when they're not controlling what happens inside with it inside the adapter?

Yes. RDFProxy expects a QueryParameters object, the route parameters are a different matter, as mentioned, you can define whatever route parameters you want. But the interface of SPARQLModelAdapter.query expects a QueryParameters object.

As mentioned, there is a scenario where subclassing QueryParameters for custom parameters makes sense: If you wanted your custom parameters to show up in Parameter model based OpenAPI docs. You could also define constraints using pydantic.Fields like the base QueryParameters model already does. Those constraints would then be enforced by FastAPI.

Again, see Query Parameter Models

@kevinstadler
Copy link

As mentioned, there is a scenario where subclassing QueryParameters for custom parameters makes sense: If you wanted your custom parameters to show up in Parameter model based OpenAPI docs.

What would the code to achieve that look like in practice?

class MyParams(QueryParameters):
    do_something_special: bool = False

@app.get("/written_work")
def written_work(params: Annotated[ MyParams, ??whatgoeshere?? ]) -> Page[WrittenWork]:

    adapter = SPARQLModelAdapter(
        target="...",
        query="...",
        model=WrittenWork,
    )
    return adapter.query(params)

@lu-pl
Copy link
Contributor

lu-pl commented Jan 14, 2025

Kevin and I discussed the case of a custom QueryParameters elsewhere, but I want to summarize it here for documentation purposes.

A primitive route definition using a custom QueryParameters object would look like this:

class MyCustomQueryParameters(QueryParameters):
    custom_parameter: int = Field(default=1000, ge=999)

@app.get("/")
def base_route(
    query_parameters: Annotated[MyCustomQueryParameters, Query()],
) -> Page[SomeModel]:
    return adapter.query(query_parameters)

This would result in the following OpenAPI docs:
image

Again, this follows the FastAPI best practice of Query Parameter models while still allowing backend implementers to instantiate QueryParameter models manually.

E.g. it is also possible to define a route with whatever arguments, as long as SPARQLModelAdapter.query is passed a parameter model. See comment above

The benefit of Query Parameter models are, 1. a well defined interface for code that processes parameters, in that case rdfproxy internals, 2. FastAPI treats parameter models specially and genereates enhanced API docs.

FastAPI uses additional arguments of typing.Annotated for triggering that behavior, as mentioned in the docs, the first argument of Annotated is for static checkers, all other args are for runtime custom processing. If FastAPI encounters a Query() object in Annotated, the route is handled specially.

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

No branches or pull requests

3 participants