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

msgspec: Optional fields are missing a default when using --snake-case-field #1920

Open
maringuu opened this issue Apr 15, 2024 · 6 comments · May be fixed by #1942
Open

msgspec: Optional fields are missing a default when using --snake-case-field #1920

maringuu opened this issue Apr 15, 2024 · 6 comments · May be fixed by #1942

Comments

@maringuu
Copy link

Steps to reproduce

  1. Download the NVD CVE schema
  2. Generate a msgpsec model:
datamodel-codegen \
    --input $schema_json \
    --input-file-type jsonschema \
    --output-model-type 'msgspec.Struct' \
   # This is important I think
    --snake-case-field \
    --output "."
  1. (Ignore the circular imports Importing generated code results in a circular dependency #836)
  2. (Ignore wrong field ordering msgspec: Mandatory attributes follow optional ones #1919
  3. Look at the class CpeMatch (and most other classes as well).
class CpeMatch(Struct, kw_only=True):
    vulnerable: bool
    criteria: str
    match_criteria_id: str = field(name='matchCriteriaId')
    version_start_excluding: Optional[str] = field(name='versionStartExcluding')
    version_start_including: Optional[str] = field(name='versionStartIncluding')
    version_end_excluding: Optional[str] = field(name='versionEndExcluding')
    version_end_including: Optional[str] = field(name='versionEndIncluding')

vs

"cpe_match": {
	"description": "CPE match string or range",
	"type": "object",
	"properties": {
		"vulnerable": {"type": "boolean"},
		"criteria": {"type": "string"},
		"matchCriteriaId": {"type": "string", "format": "uuid"},
		"versionStartExcluding": {"type": "string"},
		"versionStartIncluding": {"type": "string"},
		"versionEndExcluding": {"type": "string"},
		"versionEndIncluding": {"type": "string"}
	},
	"required": ["vulnerable", "criteria", "matchCriteriaId"],
	"additionalProperties": false
},

Note that the optional fields are missing the default=None parameter in the field call.

Expected behavior

The field should have a default of value None.

Workaround

Do not use --snake-case-field.

Setup

$ datamodel-codegen --version
0.25.5

$ python --version
Python 3.11.8
maringuu added a commit to maringuu/fkie-cve-make-release that referenced this issue May 6, 2024
@ianbuss
Copy link

ianbuss commented May 10, 2024

@maringuu https://github.com/jcrist/msgspec/blob/13a06dd88bcf553bb0497da3b3f0a2a628627ed6/msgspec/__init__.py#L20 default=None is the default for field(), so this might not be an issue in practice?

@maringuu
Copy link
Author

so this might not be an issue in practice?

No, it is an issue.
As seen in the link to the code you posted, the default is default=NODEFAULT which is not equivalent to default=None.
If a field has no default, you have to pass it to the constructor, when it should default to None.

@ianbuss
Copy link

ianbuss commented May 10, 2024

@maringuu Yes you're 100% right, I just found that out the hard way. I think I have addressed it in this PR...: #1942 (specifically in ac120c3).

@ianbuss
Copy link

ianbuss commented May 10, 2024

@maringuu Yes you're 100% right, I just found that out the hard way. I think I have addressed it in this PR...: #1942 (specifically in ac120c3).

Probable that that defaults should apply even for non-aliased fields when I think about it. Modifying my PR....

@ianbuss ianbuss linked a pull request May 10, 2024 that will close this issue
@ianbuss
Copy link

ianbuss commented May 10, 2024

See what you think of #1942 @maringuu, I think it addresses two of your issues (#1919 and #1920)

@maringuu
Copy link
Author

See what you think of #1942 @maringuu, I think it addresses two of your issues (#1919 and #1920)

Thank you for working on this!
I'll have a look at it next week if I find the time.

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