datamodel-code-generator icon indicating copy to clipboard operation
datamodel-code-generator copied to clipboard

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

Open maringuu opened this issue 10 months ago • 6 comments

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 #836)
  2. (Ignore wrong field ordering #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 avatar Apr 15 '24 12:04 maringuu

@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?

ianbuss avatar May 10 '24 11:05 ianbuss

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.

maringuu avatar May 10 '24 11:05 maringuu

@maringuu Yes you're 100% right, I just found that out the hard way. I think I have addressed it in this PR...: https://github.com/koxudaxi/datamodel-code-generator/pull/1942 (specifically in https://github.com/koxudaxi/datamodel-code-generator/pull/1942/commits/ac120c3e441e313eaf2332a2e4e10191316b101f).

ianbuss avatar May 10 '24 12:05 ianbuss

@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 avatar May 10 '24 12:05 ianbuss

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

ianbuss avatar May 10 '24 13:05 ianbuss

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.

maringuu avatar May 10 '24 16:05 maringuu