Implement easy access query parameters and request body validation based on query parameters #363

kliwongan · 2023-01-17T05:30:03Z

Description

This PR fixes #289 and may help with #351 (since msgspec encoding is apparently much faster than the standard json lib)
Query validation is optional, may find another way to indicate whether to verify (or not) instead of saving it in
the FunctionInfo, or force the change in general (when the "full" type-checking and schema validation is implemented)
The schema validation method itself supports nested classes and does not require the user to use external libraries.

The general idea of the fix is:
When a request is handled, use the Python GIL to run the steps below using the Python interpreter (possible bottleneck):

Get the signature of the model using the features in the inspect module. The current PR supports custom user-defined (potentially nested) objects in the handler function signature.
Generate a msgspec.Struct based model for schema validation based on either the member variables of the class (if it does not have a user-defined init constructor, or the constructor signature itself)
Compare the model against the received request (by decoding the JSON result into the model object) to see if the schema validation fails; if it does, an InternalServer error is raised.
If schema validation succeeds, we convert the object generated by the model validation into a dictionary and pass it to the handler function as kwargs and get the output of the handler function as usual

Further extensions:

Implement process/thread-safe caching of Encoder and Decoder objects for the msgspec encode and decode calls. Since a call to either method requires some setup, multiple calls can be made faster if we use an Encoder/Decoder object for the respective calls.

…uery parameters

fix: better handling of route return type (sparckles#349)

…ialize

netlify · 2023-01-17T05:30:07Z

✅ Deploy Preview for robyn canceled.

Name	Link
🔨 Latest commit	`437b7de`
🔍 Latest deploy log	https://app.netlify.com/sites/robyn/deploys/63f666736a810c0008d3e9ee

sansyrox

Thanks @kliwongan 😄

The PR looks nice from my first glance ✨ I think I will require some time to go through it but I have one question for now

sansyrox · 2023-01-18T21:32:06Z

dev-requirements.txt

@@ -2,7 +2,6 @@
 flake8==4.0.1
 black==21.12b0
 websockets==10.1
-maturin==0.12.11


Why did you delete this?

Ah, it was simply because installing from the dev-requirements caused an error, since the requirements.txt file already had maturin in it, and so there was a double reference to maturin. I had meant to revert it back before posting the PR and then consult someone about it.

@kliwongan , I understand now. Maybe we can just keep maturin instead of maturin==0.12.11 ? That way we won't have conflicts

kliwongan

Important to note: If possible, I intend to run the code in dependencies.py in Rust to avoid cyclical imports, however it is much easier to write in Python. If asked to do so, I will (somehow) replicate the code there in Rust without too much overhead and optimize as much as possible.
I am simply leaving it like this to demonstrate that certain features are possible, then porting over after I finalize the method in which I implement those features.

kliwongan

We may not need ForwardRefs, my current approach with user-defined classes is actually somewhat correct, and by using a simple print statement I can confirm that evaluate_forwardref is never run, but we can leave it in there for backwards compatibility for now?

Inferred from fastapi/fastapi#5461 and also PEP 563

sansyrox · 2023-01-19T23:13:41Z

src/executors/mod.rs

+        // Perform query param validation
+        let request_hashmap = request.to_hashmap(py).unwrap();
+        let handler = function.handler.as_ref(py);
+        let robyn = py.import("robyn").unwrap();


do we need to import robyn here?

sansyrox · 2023-01-19T23:24:44Z

Also @kliwongan , apologies for these one liner changes. I will do a thorough review either tomorrow or on Saturday 😄

AntoineRR · 2023-01-20T21:52:51Z

Hi @kliwongan, and thanks for the PR !
I took a look at this too as it aims to bring an important new feature to Robyn. I agree with you that the current architecture isn't the best as it implies that we use Robyn's Python code in Robyn's Rust code. This could be an issue for separating Robyn's web server and Robyn's web framework in the future AFAIK (see #268), and also decrease performances. I would suggest indeed to do the validation in Rust directly, which will probably and unfortunately be much harder 😞 Maybe we could use or be inspired by projects like pydantic-core which aims at migrating pydantic core validation to Rust ?
I don't know if @sansyrox agrees with me, but should we rewrite the data validation in Rust, I could try to help you with it 🙂

kliwongan · 2023-01-20T22:01:14Z

Hi @kliwongan, and thanks for the PR ! I took a look at this too as it aims to bring an important new feature to Robyn. I agree with you that the current architecture isn't the best as it implies that we use Robyn's Python code in Robyn's Rust code. This could be an issue for separating Robyn's web server and Robyn's web framework in the future AFAIK (see #268), and also decrease performances. I would suggest indeed to do the validation in Rust directly, which will probably and unfortunately be much harder 😞 Maybe we could use or be inspired by projects like pydantic-core which aims at migrating pydantic core validation to Rust ? I don't know if @sansyrox agrees with me, but should we rewrite the data validation in Rust, I could try to help you with it 🙂

I have a buggy branch (not sure if it's my machine or not) that somewhat addresses this problem https://github.com/kliwongan/robyn/tree/buggy-serialize; I essentially store the "validator" function in the FunctionInfo and then call it when necessary; for some reason I am getting a lot of connection errors when running tests (pytest and sending requests using Postman) and the errors are inconsistent, does anyone mind helping me investigate?

EDIT: Buggy branch works now, merged and deleted.

Anyways, if required, I would also love to rewrite the query validation in Rust! I've seen FastAPI's code and I have a good idea on how to apply other kinds of dependency injection as well (at least on the Python side). I agree on perhaps taking inspiration from pydantic-core, it might be a good idea. We can perhaps mix it in with the "validator" concept I have, where the user can also use whatever model validation function they want, which would return the **kwargs that the handler needs. However, I'm not even sure if the query validation should be a part of the server or the framework

sansyrox · 2023-01-21T13:52:51Z

integration_tests/conftest.py

+
+# Classes for testing
+class Test():
+    f: int
+    g: int
+
+class NestedCls():
+    f: Test
+    special: Optional[str] = "Nice"
+
+class TestCtor:
+    a: int
+    b: str
+
+    def __init__(self, a: int, b: str = "Nice"):
+        self.a = a
+        self.b = b
+
+class Nested:
+    c: int
+    d: str
+
+class TestQueryType:
+    a: int
+    b: str
+
+class TestForwardRef:
+    a: 'Ref'
+
+class Ref:
+    a: int
+    b: str


Hey @kliwongan , can we move this to a different file in the integration_tests folder only?

I usually tend to keep the fixtures in the conftest file and other imports can live in other files

sansyrox · 2023-01-21T13:53:31Z

dev-requirements.txt

@@ -1,8 +1,7 @@
 -r requirements.txt
 flake8==4.0.1
 black==21.12b0
-websocket-client==1.4.2


You might have to rebase as this is needed for us and was added in one of the latest requests

sansyrox · 2023-01-21T14:14:25Z

Hey @kliwongan , thank you for your great work 😄 You PR shows a lot of hard work. Especially needing you to dive deep in Pydantic and FastAPI's codebase.

But as @AntoineRR(thanks for the review btw 😄 ) suggested, we would love to have the validation written in the Rust side. As it makes the performance slower and makes it harder to segregate the responsibilities in the codebase.

Why we are emphasizing this much on writing the validation in Rust side is because we believe that we do the hard things so that the developers using Robyn don't have to. This way we will be to provide the best experience.

We are more than happy to help you out with the implementation if need be. 😄

Having said that, great work so far! ✨

AntoineRR · 2023-04-24T20:34:40Z

Hi @kliwongan, do you still have time to work on this?

kliwongan · 2023-04-26T05:58:38Z

Hi @kliwongan, do you still have time to work on this?

Yes, I'm starting to get a bit more free time now, I will restart working on it soon.

sansyrox · 2023-05-15T19:49:48Z

Hey @lecronium#4930 👋

So, I have the comments ready for the PR. I had this idea that we can allow multiple libraries for data validation. And if the there are any custom types(not the python default types), we can set validate=True by default unless someone overrides it to False.

e.g.

@app.post("/query_validation_complex", validate=True)
async def test_validation_complex(a: int, b: str, c: MsgSpecClass, d:PydanticClass):
    return jsonify({"a": a, "b": b, "c": c})

Also, we should make the data validation imports optional(maybe). Like we do with templating - To keep the dependencies as minimal as possible.

Let me know what you think of it 😄

sansyrox · 2024-04-22T20:44:45Z

Hey @kliwongan 👋

Welcome back 😄

We have two updated requirements:

Allow request members to be accessed through keyword names

-e.g. currently you can do

@app.get('/')
def fx(request):
   ....

What I am aiming for is

@app.get('/')
def fx(request, body, query_params, json, ...):
   ....

And then add validation.

Ideally, I would want the validator to be plug-and-playable and off by default. i.e. the validation overhead should be present only when the developer explicitly turns it on.

And I would want to support both pydantic and msgspec, which can be configurable.

I believe your API implementation is quite generic, which is a great thing. Would love if we can fit the above requirements in the implementation.

kliwongan and others added 10 commits January 12, 2023 21:40

Added simple default type dependency checking for/and optional easy q…

ad498dc

…uery parameters

Fixed indenting

f93f2d3

Fixed errors

3dea546

Merge pull request #1 from sansyrox/main

8bb00aa

fix: better handling of route return type (sparckles#349)

Merge branch 'main' of https://github.com/kliwongan/robyn into depends

cb05c4f

Merge branch 'sansyrox:main' into depends

eb3ae46

Added msgspec schema checking

7a55b85

Removed print statements, fixed some minor issues

5577fe8

Merge branch 'depends' of https://github.com/kliwongan/robyn into ser…

0ceafb6

…ialize

Remove unnecessary crate

f16e85c

kliwongan changed the title ~~Serialize~~ Implement easy access query parameters and request body validation based on query parameters Jan 17, 2023

sansyrox reviewed Jan 18, 2023

View reviewed changes

kliwongan commented Jan 18, 2023

View reviewed changes

Fixed item number 1

6f5b5ae

kliwongan force-pushed the serialize branch from f293e36 to 6f5b5ae Compare January 19, 2023 02:19

Merge branch 'main' into serialize

e3175a6

kliwongan commented Jan 19, 2023

View reviewed changes

kliwongan requested a review from sansyrox January 19, 2023 02:33

sansyrox mentioned this pull request Jan 19, 2023

Robyn's plan for 2023! ✨ #366

Closed

Remove extraneous println! statements

7159c86

sansyrox reviewed Jan 19, 2023

View reviewed changes

Added validator field in FunctionInfo to remove cyclic imports

4c79592

Remove extraneous print statements

500f02f

kliwongan requested a review from sansyrox January 20, 2023 23:25

sansyrox reviewed Jan 21, 2023

View reviewed changes

sansyrox mentioned this pull request Mar 9, 2023

[Feature Request] Allow dependency injection and make the objects accessible in the request object #214

Closed

sansyrox force-pushed the main branch 4 times, most recently from ee6866d to 3ef0cdb Compare March 24, 2023 09:50

sansyrox force-pushed the main branch 5 times, most recently from 52e7c30 to 1ea387f Compare April 10, 2023 04:39

sansyrox force-pushed the main branch 6 times, most recently from 7ff111d to 5588625 Compare August 11, 2023 22:51

sansyrox force-pushed the main branch from 5588625 to d04fb5e Compare August 12, 2023 22:02

sansyrox force-pushed the main branch from 8a585b8 to 2e541d9 Compare December 11, 2023 00:37

sansyrox force-pushed the main branch from f3d1c82 to ce49a2a Compare December 20, 2023 12:51

sansyrox mentioned this pull request Oct 28, 2024

Public RoadMap #1001

Open

1 task

sansyrox force-pushed the main branch 5 times, most recently from 07f28de to 0b766c9 Compare January 7, 2025 01:44

sansyrox force-pushed the main branch from 7203268 to 46880c3 Compare January 21, 2025 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement easy access query parameters and request body validation based on query parameters #363

Implement easy access query parameters and request body validation based on query parameters #363

kliwongan commented Jan 17, 2023 •

edited

Loading

netlify bot commented Jan 17, 2023 •

edited

Loading

sansyrox left a comment

sansyrox Jan 18, 2023

kliwongan Jan 18, 2023

sansyrox Jan 19, 2023

kliwongan left a comment •

edited

Loading

kliwongan left a comment •

edited

Loading

sansyrox Jan 19, 2023

sansyrox commented Jan 19, 2023

AntoineRR commented Jan 20, 2023

kliwongan commented Jan 20, 2023 •

edited

Loading

sansyrox Jan 21, 2023

sansyrox Jan 21, 2023

sansyrox commented Jan 21, 2023 •

edited

Loading

AntoineRR commented Apr 24, 2023

kliwongan commented Apr 26, 2023

sansyrox commented May 15, 2023 •

edited

Loading

sansyrox commented Apr 22, 2024

Implement easy access query parameters and request body validation based on query parameters #363

Are you sure you want to change the base?

Implement easy access query parameters and request body validation based on query parameters #363

Conversation

kliwongan commented Jan 17, 2023 • edited Loading

netlify bot commented Jan 17, 2023 • edited Loading

✅ Deploy Preview for robyn canceled.

sansyrox left a comment

Choose a reason for hiding this comment

sansyrox Jan 18, 2023

Choose a reason for hiding this comment

kliwongan Jan 18, 2023

Choose a reason for hiding this comment

sansyrox Jan 19, 2023

Choose a reason for hiding this comment

kliwongan left a comment • edited Loading

Choose a reason for hiding this comment

kliwongan left a comment • edited Loading

Choose a reason for hiding this comment

sansyrox Jan 19, 2023

Choose a reason for hiding this comment

sansyrox commented Jan 19, 2023

AntoineRR commented Jan 20, 2023

kliwongan commented Jan 20, 2023 • edited Loading

sansyrox Jan 21, 2023

Choose a reason for hiding this comment

sansyrox Jan 21, 2023

Choose a reason for hiding this comment

sansyrox commented Jan 21, 2023 • edited Loading

AntoineRR commented Apr 24, 2023

kliwongan commented Apr 26, 2023

sansyrox commented May 15, 2023 • edited Loading

sansyrox commented Apr 22, 2024

kliwongan commented Jan 17, 2023 •

edited

Loading

netlify bot commented Jan 17, 2023 •

edited

Loading

kliwongan left a comment •

edited

Loading

kliwongan left a comment •

edited

Loading

kliwongan commented Jan 20, 2023 •

edited

Loading

sansyrox commented Jan 21, 2023 •

edited

Loading

sansyrox commented May 15, 2023 •

edited

Loading