please review

CodSpeed Performance Report

Merging#10478 willnot alter performance

_{ComparingFlorianSW:feature/4393 (dd77f91) withmain (c9c9277)}

Summary

✅ 38 untouched benchmarks

Sorry for the additional commits, I haven't tested any other python version than 3.12, which I've installed locally 😬

Coverage report

This PR does not seem to contain any modification to coverable code.

Agreed with@Viicos' analysis here.

Thanks for the reviews :) I added the changes in the latest commit.

Thanks for all your support and the reviews 🙏
I added the comments into the PR :)

Ah, I see why my change broke things. Need to think a bit more about this.

There's a part of me that thinks this should be more unconditional, like:

json_schema:JsonSchemaValue= {'type':'object'}keys_schema=self.generate_inner(schema['keys_schema']).copy()if'keys_schema'inschemaelse {}keys_pattern=keys_schema.pop('pattern',None)values_schema=self.generate_inner(schema['values_schema']).copy()if'values_schema'inschemaelse {}values_schema.pop('title',None)# don't give a title to the additionalPropertiesifvalues_schemaorkeys_patternisnotNone:# don't add additionalProperties if it's emptyifkeys_patternisNone:json_schema['additionalProperties']=values_schemaelse:json_schema['patternProperties']= {keys_pattern:values_schema}keys_schema.pop('title',None)# don't give a title to the propertyNamesifkeys_schema:json_schema['propertyNames']=keys_schema

And we can just use refs like we would for other types. Need to chat with@Viicos before we move forward here.

Thanks for your help@FlorianSW, we can take it from here :).

@sydney-runkle, with your proposed change,propertyNames will be set even for keys with a type different than string?

classModel(BaseModel):f:dict[Annotated[int,Field(gt=1)],str]Model(f={2:"a"}).model_dump_json()#> '{"f": {"2": "a"}}'

While such keys are converted to strings when dumping to JSON, we shouldn't add apropertyNames schema if it isn't of typestring (and anyway,gt=1 isn't a constraint that can be expressed for strings -- or maybe by mapping it to a regex constraint but this is a different topic).

The len check feels a bit hacky, but I don't know if there's a better way to do so 🤔

Ah I see, I'm alright with just adding forstring types then.

I'm just hesitant to eagerly resolve the schema refs here - we don't do that elsewhere, and I'm afraid we'll run into errors with available defs at this point in the schema building process (I suppose we can fix and ignore with a catch, though that seems like an annoyingly silent failure).

Thoughts on this@Viicos?

So, I'm ok with this resolution ahead of time, as we do this for discriminator refs eagerly :).

See