Fix const validations #794

hmvp · 2019-09-06T13:08:28Z

This fixes #620

NOTICE: pydantic is currently pushing towards V1 release,
see issue 576. Changes not required to release version 1
may be be delayed until after version 1 is released. If your PR is a bugfix for v0.32, please base off and target the v0.32.x branch.

Change Summary

Related issue number

Checklist

Unit tests for the changes exist
Tests pass on CI and coverage remains at 100%
Documentation reflects the changes where applicable
changes/<pull request or issue id>-<github username>.rst file added describing change
(see changes/README.md for details)

codecov · 2019-09-06T13:27:32Z

Codecov Report

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

@@          Coverage Diff           @@
##           master   #794    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files          17     17            
  Lines        2984   2847   -137     
  Branches      580    559    -21     
======================================
- Hits         2984   2847   -137

Impacted Files	Coverage Δ
pydantic/fields.py	`100% <100%> (ø)`	⬆️
pydantic/error_wrappers.py	`100% <100%> (ø)`	⬆️
pydantic/types.py	`100% <0%> (ø)`	⬆️
pydantic/errors.py	`100% <0%> (ø)`	⬆️
pydantic/typing.py	`100% <0%> (ø)`	⬆️
pydantic/networks.py	`100% <0%> (ø)`	⬆️
pydantic/validators.py	`100% <0%> (ø)`	⬆️
pydantic/schema.py	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b467da...e6d6389. Read the comment docs.

hmvp · 2019-09-06T13:33:52Z

Not sure if you wan't this in master or 0.32 so I submitted both

samuelcolvin

looks like a good start, please can you also fix coverage.

pydantic/fields.py

HISTORY.rst

tests/test_main.py

samuelcolvin · 2019-09-06T15:39:34Z

does this also resolve #793?

hmvp · 2019-09-07T09:15:19Z

does this also resolve #793?

No, unfortunately it doesn't, and I don't really have a clue how to fix it. However it might not be a big issue for us since we initialize the constant fields with json compatible dicts instead of submodel instances

hmvp · 2019-09-07T09:43:36Z

does this also resolve #793?

No, unfortunately it doesn't, and I don't really have a clue how to fix it. However it might not be a big issue for us since we initialize the constant fields with json compatible dicts instead of submodel instances

Ok changed it up a bit more and now it should work for #793. At least in the most obvious cases.

It still fails if you initialize the default value with a BaseModel instance instead of a dict.

samuelcolvin

Looking good to me, just a few small things to change.

@tiangolo, please could you review this since you did most of the original work on schema.

samuelcolvin · 2019-09-09T09:11:13Z

tests/test_main.py

+
+    m = Model()
+    assert m.a == [SubModel(b=1), SubModel(b=2), SubModel(b=3)]
+    assert m.b == [SubModel(b=4), SubModel(b=5), SubModel(b=6)]


would it also help to add m.schema() == {...} here?

Added, So this is interesting:

It seems that adding SubModel instances as default breaks in a lot of ways: Also the schema is now not json serializable, similar to the error as per #793 .

It might be a good idea to just document or add a warning of some sorts for that case and consider that unsupported. Of course not all use cases need json serializability and some use cases might want to set the default using a SubModel instance.

please create an issue about this.

samuelcolvin · 2019-09-09T09:13:48Z

tests/test_main.py

+    try:
+        # Fails
+        Form(field1={'field': 1}, field2=[{'field': 1}])
+    except ValidationError as e:


use pytest.raises(ValidationError) as exc_info:.... exc_info.e.json()

samuelcolvin · 2019-09-09T09:14:30Z

tests/test_main.py

+        Form(field1={'field': 1}, field2=[{'field': 1}])
+    except ValidationError as e:
+        # This should not raise an Json error
+        assert json.dumps(e.json())


this doesn't make sense, e.json() returns a string, this is effectively just json.dumps('foo') -> '"foo"'

samuelcolvin · 2019-09-17T11:40:23Z

Hi @hmvp would be great if you could work on fixing this. I'm trying to get a beta for version 1 out the door asap and I want to include this.

hmvp · 2019-09-17T11:41:23Z

Sure, I will try to get a fixed MR out today

This fixes pydantic#620 and pydantic#793

hmvp · 2019-09-17T12:47:11Z

Pushed an update

samuelcolvin · 2019-09-17T17:29:38Z

pydantic/fields.py

-        if getattr(self.type_, '__origin__', None):
-            # type_ has been refined eg. as the type of a List and sub_fields needs to be populated
-            self.sub_fields = [self._create_sub_type(self.type_, '_' + self.name)]
+        # type_ has been refined eg. as the type of a List and sub_fields needs to be populated


I've been working a lot on this part of the code in #803, I'm now sure this code will need more work.

But maybe it would be easier for me to fix it in #803.

samuelcolvin · 2019-09-17T17:29:41Z

tests/test_main.py

+
+    m = Model()
+    assert m.a == [SubModel(b=1), SubModel(b=2), SubModel(b=3)]
+    assert m.b == [SubModel(b=4), SubModel(b=5), SubModel(b=6)]


please create an issue about this.

hmvp force-pushed the fix_const_validations branch 2 times, most recently from 4a6b5cf to f964990 Compare September 6, 2019 13:27

samuelcolvin reviewed Sep 6, 2019

View reviewed changes

pydantic/fields.py Outdated Show resolved Hide resolved

pydantic/fields.py Outdated Show resolved Hide resolved

HISTORY.rst Outdated Show resolved Hide resolved

tests/test_main.py Outdated Show resolved Hide resolved

hmvp force-pushed the fix_const_validations branch from f964990 to fb08c18 Compare September 7, 2019 09:05

hmvp force-pushed the fix_const_validations branch from fb08c18 to 43152ee Compare September 7, 2019 09:40

hmvp force-pushed the fix_const_validations branch from 43152ee to 9260975 Compare September 7, 2019 09:47

samuelcolvin reviewed Sep 9, 2019

View reviewed changes

samuelcolvin requested review from tiangolo and removed request for tiangolo September 9, 2019 09:17

Fix const validations

e6d6389

This fixes pydantic#620 and pydantic#793

hmvp force-pushed the fix_const_validations branch from 9260975 to e6d6389 Compare September 17, 2019 12:17

samuelcolvin approved these changes Sep 17, 2019

View reviewed changes

samuelcolvin merged commit dd32a43 into pydantic:master Sep 17, 2019

hmvp deleted the fix_const_validations branch January 6, 2020 14:53

alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023

Remove patch version number from error link (pydantic#794)

ec6f719

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix const validations #794

Fix const validations #794

hmvp commented Sep 6, 2019 •

edited

Loading

codecov bot commented Sep 6, 2019 •

edited

Loading

hmvp commented Sep 6, 2019

samuelcolvin left a comment

samuelcolvin commented Sep 6, 2019

hmvp commented Sep 7, 2019

hmvp commented Sep 7, 2019 •

edited

Loading

samuelcolvin left a comment

samuelcolvin Sep 9, 2019

hmvp Sep 17, 2019

samuelcolvin Sep 17, 2019

samuelcolvin Sep 9, 2019

hmvp Sep 17, 2019

samuelcolvin Sep 9, 2019

hmvp Sep 17, 2019

samuelcolvin commented Sep 17, 2019

hmvp commented Sep 17, 2019

hmvp commented Sep 17, 2019

samuelcolvin Sep 17, 2019

samuelcolvin Sep 17, 2019

Fix const validations #794

Fix const validations #794

Conversation

hmvp commented Sep 6, 2019 • edited Loading

Change Summary

Related issue number

Checklist

codecov bot commented Sep 6, 2019 • edited Loading

Codecov Report

hmvp commented Sep 6, 2019

samuelcolvin left a comment

Choose a reason for hiding this comment

samuelcolvin commented Sep 6, 2019

hmvp commented Sep 7, 2019

hmvp commented Sep 7, 2019 • edited Loading

samuelcolvin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelcolvin commented Sep 17, 2019

hmvp commented Sep 17, 2019

hmvp commented Sep 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmvp commented Sep 6, 2019 •

edited

Loading

codecov bot commented Sep 6, 2019 •

edited

Loading

hmvp commented Sep 7, 2019 •

edited

Loading