Skip to content

Conversation

jparise
Copy link

@jparise jparise commented Oct 22, 2022

Support for patternProperties was introduced in #332, but that logic unfortunately made patternProperties and additionalProperties mutually exclusive. JSON Schema supports their combination:

https://json-schema.org/understanding-json-schema/reference/object.html#additional-properties

This prevented well-typed code generation for dictionaries that use regex-constrained string keys.

@jparise
Copy link
Author

jparise commented Oct 22, 2022

This patch is for the v1.10.x branch. The main (v2) patch is #4642.

Support for `patternProperties` was introduced in pydantic#332, but that logic
unfortunately made `patternProperties` and `additionalProperties`
mutually exclusive. JSON Schema supports their combination:

https://json-schema.org/understanding-json-schema/reference/object.html#additional-properties

This prevented well-typed code generation for dictionaries that use
regex-constrained string keys.
@jparise
Copy link
Author

jparise commented Oct 22, 2022

The failing FastAPI test in this branch appears to be unrelated to this change. It's dying in an SQLAlchemy code path (sqlalchemy/engine/cursor.py:164: ValueError).

@samuelcolvin samuelcolvin merged commit 36be4c9 into pydantic:1.10.X-fixes Oct 27, 2022
@samuelcolvin
Copy link
Member

thanks so much.

@jparise jparise deleted the dict-properties branch October 27, 2022 23:12
@matejetz
Copy link

matejetz commented Mar 19, 2023

i think this introduced inconsistency regarding the json schema vs pydantic validation behavior.

in the following, pydantic rightfully raises a ValidationError

class Model(BaseModel):
    a: Dict[constr(regex="^text$"), str]

Model(a={'foo': 'bar'})  # correctly raises ValidationError

however, the json schema produced by Model.schema() will now validate the following, as 'additionalProperties': { 'type': 'string'} just allows any string properties.

{
  "a": {
    "foo": "bar"
  }
}

i also doubt this is the expected behavior when using a regex-constrained dict key. instead, imho, additionalProperties should be false by default and might possibly be changed by passing some param to Field.

@samuelcolvin
Copy link
Member

What version are you using? Might be worth trying on main.

We can't really make a change to this in v1, but we can get it right in V2.

@matejetz
Copy link

sorry, i forgot to include that my comment concerns v1, which this PR was also targeting.
why do you think you can't change this, i'd almost go as far and classify this as a bug or at least unwanted/ unexpected behavior.

i did not have a chance to take a look at v2 yet but will definitely do, looks very promising. love the lib! 👍

@ElectronicRU
Copy link

Yes, this seems like a wrong change to me - it makes JSON schema more permissive than the actual schema is. Can @samuelcolvin elaborate why that's not changeable in v1?

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.

4 participants