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

Add support for msgspec injection #197

Merged
merged 7 commits into from
Jul 9, 2023
Merged

Add support for msgspec injection #197

merged 7 commits into from
Jul 9, 2023

Conversation

dmckeone
Copy link
Contributor

I've recently been looking at msgspec to see if it can function as a dataclass replacement for a future Sanic project with dependency injection, as listed in the docs

I don't know if this integration is a good idea or not, but since I did the work to explore the idea, I thought I'd show it for commentary.

This change did require a number of specialized checks, and I attempted to match the existing style as closely as possible. test_validation_msgspec.py is just a copy of test_validation_attrs.py, but with msgspec.Struct instead of @define. Please let me know if there is a better testing strategy there.

Also, there is currently an open issue at msgspec #316, which would align msgspec with attrs and perhaps make this PR unnecessary.

Finally, there may be future changes needed to fully take advantage of the JSON decoding in msgspec to exploit all of its performance improvements:

@ahopkins
Copy link
Member

This seems reasonable and on the surface looks like it is pretty easy to add. Thanks for taking the time on this. Sorry for my delay in responding.

@ahopkins
Copy link
Member

Okay, just got done reading their docs. I'm pretty sure I saw this before but never looked at it much. I must say, I'm very impressed with msgspec. I really like the way it operates and all the config as class arguments is much more comfortable to me than Pydantic's Config class. I wonder how extensible it is 🤔

I'm sold on the idea of including it. Now let's look at some code 😎

ahopkins
ahopkins previously approved these changes May 30, 2023
@ahopkins
Copy link
Member

I'm wondering if this PR should go one step further and also add some sort of convenience way to update the serialization. With the idea you can just do this:

import msgspec

class User(msgspec.Struct):
    """A new type describing a User"""
    name: str
    groups: set[str] = set()
    email: str | None = None

@app.get("/alice")
async def handler(_):
    alice = User("alice", groups={"admin", "engineering"})
    return json(alice)

Albeit, that is a bit scope creep for here and should probably be its own PR.

@dmckeone
Copy link
Contributor Author

Glad to see you like it! I've been pretty impressed with msgspec in the limited time I've been using it.

I'll get that tox test passing, and I'd be happy to submit a PR for the json() function over on the sanic main project.

@ahopkins
Copy link
Member

ahopkins commented May 30, 2023

Ehh, I don't think it belongs in the main. I'm not sure what the best api is. Maybe something like this:

from sanic_ext.extras.serializer.msgspec import dumps

app = Sanic(..., dumps=dumps)

The idea being its a function in the extras that wraps all the logic needed to create the appropriate encoder/decoder.

Copy link

@jcrist jcrist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, msgspec author here. I'd be very happy to see this integration in place, happy to help support review as needed :).

sanic_ext/extras/validation/check.py Outdated Show resolved Hide resolved
@dmckeone
Copy link
Contributor Author

dmckeone commented Jun 3, 2023

I've updated the tox environment and added @jcrist's suggestion above. Message received on the serializer side. I'll create a PR on sanic-ext when I get some more time to look at it.

@ahopkins
Copy link
Member

ahopkins commented Jul 9, 2023

For json data, it'd be nice to parse the JSON payload directly into the requested type, avoiding any unnecessary intermediate allocations. I'm not sure if sanic/sanic-ext are architected in a way that would enable this currently, but enabling this would be the most performant option.

To @jcrist point, this is something that generally is being handled in the core Sanic project. Sanic provides a few ways to pass a custom loads method. We can probably take this up in a different thread, but what I would like to see is some convenience funcs to facilitate something like this. Of course alternative API suggestions welcome.

from sanic_ext.extras.msgspec import loads as msgspec_loads
from sanic_ext import validate

app = Sanic(..., loads=msgspec_loads)

class Foo(Struct):
    ...

@app.post("/foo")
@validate(json=Foo)
async def handler(request: Request, body: Foo):
    ...

ahopkins
ahopkins previously approved these changes Jul 9, 2023
ahopkins
ahopkins previously approved these changes Jul 9, 2023
ahopkins
ahopkins previously approved these changes Jul 9, 2023
ahopkins
ahopkins previously approved these changes Jul 9, 2023
@ahopkins ahopkins merged commit 38a8964 into sanic-org:main Jul 9, 2023
4 checks passed
@dmckeone dmckeone deleted the msgspec-injection branch July 12, 2023 23:28
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

Successfully merging this pull request may close these issues.

3 participants