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

DM-46002: Locations for validation errors on constraints reported incorrectly #102

Merged
merged 9 commits into from
Aug 29, 2024

Conversation

JeremyMcCormick
Copy link
Collaborator

@JeremyMcCormick JeremyMcCormick commented Aug 28, 2024

This fixes an issue where the Pydantic error locations for constraint objects were reported incorrectly, due to how the model objects were constructed.

Instead of using a custom create_constraints() function, a Pydantic discriminated union was used to automatically construct the objects based on the type field. This seems to fix the issue where error locations were reported incorrectly.

Sample new error location is: tables.0.constraints.0.Unique.@id.

This is not exactly the same scheme that shows for some other types of objects, but it is at least clear now that the error has occurred in a specific constraint denoted by the index.

As a result of the above changes, usage of the type field in the metadata and tap modules was removed in favor of checking the constraint's type using isinstance(). This is required based on removal of the type field from the Constraint class. (Since it should never be instantiated, the type field is not needed on this class and including it creates typing conflicts with its child classes. Each constraint class now defines this separately and not as a common field from the parent class.)

A few additional changes were included which do not have associated tickets:

  • Cleaned up the Constraint test case in test_datamodel (See 1706788).
  • Added check to the data model that the deferrable field on Constraint must be set to True if initially is not None. This requirement was indicated in comments but never implemented.
  • Added a Literal to Constraint in the data model that limits the value of initially to IMMEDIATE or DEFERRED as these are the only valid values for an INITIALLY clause in DDL.
  • Removed the unused annotations field on the Constraint class. (This field was erroneously carried over from the old "simple model" but has never been used within a production schema, to my knowledge.)
  • Cleaned up the Index test case in test_datamodel (See 6631610).

Checklist

  • Ran Jenkins
  • Added a release note for user-visible changes to docs/changes

@JeremyMcCormick JeremyMcCormick marked this pull request as draft August 28, 2024 19:40
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 98.83721% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.01%. Comparing base (364fc5c) to head (f8b514d).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
python/felis/metadata.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
+ Coverage   90.92%   91.01%   +0.08%     
==========================================
  Files          16       16              
  Lines        1885     1881       -4     
  Branches      417      410       -7     
==========================================
- Hits         1714     1712       -2     
+ Misses         97       96       -1     
+ Partials       74       73       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JeremyMcCormick JeremyMcCormick marked this pull request as ready for review August 28, 2024 22:08
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-46002 branch 2 times, most recently from 0b089d2 to 5329827 Compare August 28, 2024 22:27
Copy link

@gpdf gpdf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit to not being qualified to review the Pydantic details involved here; the goals seem appropriate and I'm willing to accept that the resulting messages are improved in their intelligibility.

Replace the create_constraints() function with a discriminated union:

https://docs.pydantic.dev/latest/concepts/unions/#discriminated-unions-with-str-discriminators

This fixes the issue where the locations reported for errors on
constraints were incorrect.
The SQL INITIALLY clause may only take the values "IMMEDIATE" and
"DEFERRED" so constrain the field to these values in the Pydantic
model.
Remove duplicate checks on name and id values.

Normalize the object names and ids to snakecase.

Add a function to test the base Constraint object, which is never
instantiated, but has deferrable and initially that need to be checked.

Move checks on name and id to the test case for the base class.

Fix some erroneous variable names.
After switching to a Pydantic discriminated union, the constraint type
does not need to be checked here. Instead the object type can be used
directly.
@JeremyMcCormick JeremyMcCormick merged commit af24244 into main Aug 29, 2024
15 checks passed
@JeremyMcCormick JeremyMcCormick deleted the tickets/DM-46002 branch August 29, 2024 01:40
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.

2 participants