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

Support for Field and extra #236

Closed
Goldziher opened this issue Dec 9, 2022 · 6 comments · Fixed by #255
Closed

Support for Field and extra #236

Goldziher opened this issue Dec 9, 2022 · 6 comments · Fixed by #255

Comments

@Goldziher
Copy link

Goldziher commented Dec 9, 2022

Hi again,

In pydantic I can pass any kwargs to a Field and these will be set as a dictionary under the ModelField.field_info.extra dictionary (if extra is allowed). I can also pass an extra dict directly. This is useful for library authors such as myself because it allows us to pass meta data as part of a field defintion.

For example, we have stuff like this: https://starlite-api.github.io/starlite/usage/3-parameters/3-the-parameter-function/.

At present the closest thing possible with msgspec is to use the Meta object with Annotated, and hijack the extra_json_schema dictionary. This though is not a proper solution - first, because this is not about an extra json schema but rather about passing kwargs, and secondly because this is not necessarily about constraints and the usage of Annotated might be redundant in this case.

@jcrist
Copy link
Owner

jcrist commented Dec 14, 2022

This is useful for library authors such as myself because it allows us to pass meta data as part of a field defintion.

Couldn't you have Parameter return a custom type that stored all the metadata on it you want, then translate that internally to Annotated[..., Meta(...)] as needed? IIUC the starlite internals properly, there's nothing that binds Parameter to returning a pydantic type, and the object returned by Parameter is opaque to users. Do you really need a change to msgspec here?

This though is not a proper solution - first, because this is not about an extra json schema but rather about passing kwargs

You're currently using Pydantic's extra arg for this, which pydantic interprets as being part of its generated JSON schema. The extra_json_schema has the same meaning in msgspec, it's just more clearly named.

...
:param **extra: any additional keyword arguments will be added as is to the schema
...
In [4]: class Model(pydantic.BaseModel):
   ...:     field: int = pydantic.Field(a_custom_key=1)
   ...: 

In [5]: Model.schema()  # view the JSON schema as generated by Pydantic
Out[5]: 
{'title': 'Model',
 'type': 'object',
 'properties': {'field': {'title': 'Field',
   'a_custom_key': 1,
   'type': 'integer'}},
 'required': ['field']}

and secondly because this is not necessarily about constraints and the usage of Annotated might be redundant in this case.

Meta is for anything that augments the schema that isn't either the type or a default value. I'm not sure what you mean by "redundant", but if you're saying you can't nest Annotated types, that's not accurate.

In [1]: from msgspec import Meta

In [2]: from typing import Annotated

In [3]: inner = Annotated[int, Meta(gt=0)]  # an existing type annotation

In [4]: outer = Annotated[inner, Meta(lt=10)]  # wrap an existing annotation in another Annotated

In [5]: import msgspec

In [6]: msgspec.json.decode('1', type=outer)
Out[6]: 1

In [7]: msgspec.json.decode('100', type=outer)  # this should error
---------------------------------------------------------------------------
ValidationError                           Traceback (most recent call last)
Cell In [7], line 1
----> 1 msgspec.json.decode('100', type=outer)  # this should error

ValidationError: Expected `int` <= 9

@Goldziher
Copy link
Author

Well, I can do a lot. The question really is why not support this and require the user to do all sorts of manipulation.

I gave Parameter as one example, I have other uses for extra as well.

Dataclasses expose an metadata dictionary that the user can store values in. Pydantic exposes the extra dictionary. It is simple to work with and generic. Basically you can pass extra kwargs to a pydantic model and these are stored in the extra dictionary. This is very useful in many places.

You can decide not to support this feature, but really I don't see why it would hurt msgspec to do this.

@jcrist
Copy link
Owner

jcrist commented Dec 18, 2022

Pydantic exposes the extra dictionary.

As I said above, the extra dictionary in Pydantic is the same as the extra_json_schema dictionary in msgspec (ours is just more clearly named). Pydantic inlines anything in **extra as part of its generated json-schema, same as we do here.

You can decide not to support this feature, but really I don't see why it would hurt msgspec to do this.

I'm not for or against this, I'm mostly confused what you're asking for here. I think part of the issue is that you're asking for a presumed solution, rather than clearly stating the problem you currently have (an example of the XY Problem).

I gave Parameter as one example, I have other uses for extra as well.
...
This is very useful in many places.

Can you list those use cases here? Knowing what you intend to use this for would be useful when designing a new API.


Perhaps it'd be easier if you show the code you'd like to write. What would that code look like?

For example, how would this new feature look if you used it in the following struct definition?

from msgspec import Struct, Meta
from typing import Annotated

class User(Struct):
    name: Annotated[str, Meta(min_length=3, max_length=32)]
    groups: list[str] | None = None
    email: str | None = None

@jcrist
Copy link
Owner

jcrist commented Jan 3, 2023

Bump @Goldziher - any response to the above? Would a new extra field in Meta that takes an arbitrary user-defined dict support your use case?

@jcrist
Copy link
Owner

jcrist commented Jan 4, 2023

I've gone ahead and added an extra field to Meta in #255. Please let me know if that's sufficient for your use case.

@Goldziher
Copy link
Author

Goldziher commented Jan 4, 2023

Yes, that would be what we need. Cheers!

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 a pull request may close this issue.

2 participants