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

Add support for ConstrainedStr as dict keys #332

Merged
merged 5 commits into from Dec 27, 2018

Conversation

Projects
None yet
3 participants
@tiangolo
Copy link
Contributor

tiangolo commented Dec 27, 2018

Change Summary

Add support for ConstrainedStr as dict keys.

Related issue number

#329

Performance Changes

pydantic cares about performance, if there's any risk performance changed on this PR,
please run make benchmark-pydantic before and after the change:

  • before: ?
  • after: ?

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes
  • No performance deterioration (if applicable)
  • HISTORY.rst has been updated
    • if this is the first change since a release, please add a new section
    • include the issue number or this pull request number #<number>
    • if you're not a regular contributer please include your github username @whatever
@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 27, 2018

Codecov Report

Merging #332 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #332   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          13     13           
  Lines        1779   1783    +4     
  Branches      346    347    +1     
=====================================
+ Hits         1779   1783    +4
@tiangolo

This comment has been minimized.

Copy link
Contributor Author

tiangolo commented Dec 27, 2018

@samuelcolvin should I rebase to solve HISTORY conflicts?

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Dec 27, 2018

Yes, or merge with master.

@tiangolo tiangolo force-pushed the tiangolo:dict-constr-key branch from c39c241 to a267c57 Dec 27, 2018

@samuelcolvin
Copy link
Owner

samuelcolvin left a comment

Otherwise LGTM.

else:
# The dict values are Any, no need to declare it
return {'type': 'object'}, definitions
if key_pattern:

This comment has been minimized.

@samuelcolvin

samuelcolvin Dec 27, 2018

Owner

no need for key_pattern just if regex here.

This comment has been minimized.

@tiangolo

tiangolo Dec 27, 2018

Author Contributor

Agreed. Done.

if key_pattern:
# Dict keys have a regex pattern
# f_schema might be a schema or empty dict, add it either way
dict_schema.update({'patternProperties': {key_pattern: f_schema}})

This comment has been minimized.

@samuelcolvin

samuelcolvin Dec 27, 2018

Owner

since we're only setting on item use setitem dict_schema['patternProperties'] here and below.

This comment has been minimized.

@tiangolo

tiangolo Dec 27, 2018

Author Contributor

Agreed. Done.

# Dict keys have a regex pattern
# f_schema might be a schema or empty dict, add it either way
dict_schema['patternProperties'] = {regex.pattern: f_schema}
elif f_schema:

This comment has been minimized.

@samuelcolvin

samuelcolvin Dec 27, 2018

Owner

Sorry, one more question.

Are these two cases really mutually exclusive? Eg. requiring elif rather than if.

maybe you might want both patternProperties and additionalProperties?

This comment has been minimized.

@tiangolo

tiangolo Dec 27, 2018

Author Contributor

Yep. I thought about the same while implementing it.

The schema that would go in additionalProperties ends up as the value of the dict at patternProperties. That dict has as key the regex pattern, and as value the schema for the dict value.


Also from the spec:

Validation with "additionalProperties" applies only to the child values of instance names that do not match any names in "properties", and do not match any regular expression in "patternProperties".

This comment has been minimized.

@demosdemon

demosdemon Dec 27, 2018

I can think of a case that would allow for patternProperties and additionalProperties, but it is rather complex.

>>> from typing import Union, Dict, Any
>>> from pydantic import BaseModel, constr
>>>
>>> Identifier = constr(regex=r'^([a-zA-Z_][a-zA-Z0-9_]*)$')
>>> Slug = constr(regex=r'^([a-zA-Z_-][a-zA-Z0-9_-]*)$')
>>> Number = constr(regex=r'^\d+$')
>>>
>>> class IdentifierModel(BaseModel):
...     alias: str = ...
...
>>> class SlugModel(BaseModel):
...     alias: str = ...
...     value: str = ...
...
>>> class NumberModel(BaseModel):
...     alias: str = ...
...     value: int = ...
...
>>> class Model(BaseModel):
...     items: Union[Dict[Identifier, IdentifierModel], Dict[Slug, SlugModel], Dict[Number, NumberModel], Dict[str, Any]] = {}
...
>>> print(Model.schema_json(indent=2))
{
  "title": "Model",
  "type": "object",
  "properties": {
    "items": {
      "title": "Items",
      "default": {},
      "anyOf": [
        {
          "type": "object",
          "patternProperties": {
            "^([a-zA-Z_][a-zA-Z0-9_]*)$": {
              "$ref": "#/definitions/IdentifierModel"
            }
          }
        },
        {
          "type": "object",
          "patternProperties": {
            "^([a-zA-Z_-][a-zA-Z0-9_-]*)$": {
              "$ref": "#/definitions/SlugModel"
            }
          }
        },
        {
          "type": "object",
          "patternProperties": {
            "^\\d+$": {
              "$ref": "#/definitions/NumberModel"
            }
          }
        },
        {
          "type": "object"
        }
      ]
    }
  },
  "definitions": {
    "IdentifierModel": {
      "title": "IdentifierModel",
      "type": "object",
      "properties": {
        "alias": {
          "title": "Alias",
          "type": "string"
        }
      },
      "required": [
        "alias"
      ]
    },
    "SlugModel": {
      "title": "SlugModel",
      "type": "object",
      "properties": {
        "alias": {
          "title": "Alias",
          "type": "string"
        },
        "value": {
          "title": "Value",
          "type": "string"
        }
      },
      "required": [
        "alias",
        "value"
      ]
    },
    "NumberModel": {
      "title": "NumberModel",
      "type": "object",
      "properties": {
        "alias": {
          "title": "Alias",
          "type": "string"
        },
        "value": {
          "title": "Value",
          "type": "integer"
        }
      },
      "required": [
        "alias",
        "value"
      ]
    }
  }
}
# expected
{
  "title": "Model",
  "type": "object",
  "properties": {
    "items": {
      "title": "Items",
      "default": {},
      "patternProperties": {
        "^([a-zA-Z_][a-zA-Z0-9_]*)$": {
          "$ref": "#/definitions/IdentifierModel"
        },
        "^([a-zA-Z_-][a-zA-Z0-9_-]*)$": {
          "$ref": "#/definitions/SlugModel"
        },
        "^\\d+$": {
          "$ref": "#/definitions/NumberModel"
        }
      },
      "additionalProperties": {},
    }
  },
  "definitions": {
    "IdentifierModel": {
      "title": "IdentifierModel",
      "type": "object",
      "properties": {
        "alias": {
          "title": "Alias",
          "type": "string"
        }
      },
      "required": [
        "alias"
      ]
    },
    "SlugModel": {
      "title": "SlugModel",
      "type": "object",
      "properties": {
        "alias": {
          "title": "Alias",
          "type": "string"
        },
        "value": {
          "title": "Value",
          "type": "string"
        }
      },
      "required": [
        "alias",
        "value"
      ]
    },
    "NumberModel": {
      "title": "NumberModel",
      "type": "object",
      "properties": {
        "alias": {
          "title": "Alias",
          "type": "string"
        },
        "value": {
          "title": "Value",
          "type": "integer"
        }
      },
      "required": [
        "alias",
        "value"
      ]
    }
  }
}

This comment has been minimized.

@samuelcolvin

samuelcolvin Dec 27, 2018

Owner

Good find, however I think this is too niche for now, if you think it's a problem @demosdemon please create a new issue to discuss.

@samuelcolvin samuelcolvin merged commit c6d1a69 into samuelcolvin:master Dec 27, 2018

3 checks passed

codecov/project 100% (+0%) compared to 7279178
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@tiangolo tiangolo deleted the tiangolo:dict-constr-key branch Dec 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.