-
Notifications
You must be signed in to change notification settings - Fork 13
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
Inspect violated constraints #224
base: master
Are you sure you want to change the base?
Conversation
|
||
error = {'code': 'invalid_data.conflict'} | ||
if column_names: | ||
error['source'] = {'pointer': '/data/{}'.format(column_names[-1])} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll actually have to go through the schema and figure out which deserializer field corresponds to this column.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to tweak the whole path thing, too. Generally the insert is only on the main model (i.e. no relationship inserts, most likely), but... eh.
|
||
error = {'code': 'invalid_data.conflict'} | ||
if column_names: | ||
error['source'] = {'pointer': '/data/{}'.format(column_names[-1])} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For multi-column unique constraints (e.g. tuple of tenant_id
, name
), we generally want to report the conflict on the last column, but this is something we need to tweak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that assumption would require some level of documentation. Alternatively could make the default to show all columns and then have a hook in the view to allow the view to reduce what is shown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs for sure. the issue is that we have a lot of unique constraints like (tenant_id, name)
, and it would be odd to show an error on both fields in such cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could also have a pointers
field being an array of pointers. we are already not jsonapi compliant anyway
table_name = diag.table_name | ||
constraint_name = diag.constraint_name | ||
|
||
insp = sa.inspect(self.session.bind) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make more sense to do this at startup for each table, or to cache these?
schema=schema_name, | ||
) | ||
|
||
for unique_constraint in unique_constraints: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... so you can make this a map based on name. not sure if all this is prem optimization, but I think makes also the code cleaner. for with breaks smells to me
|
||
error = {'code': 'invalid_data.conflict'} | ||
if column_names: | ||
error['source'] = {'pointer': '/data/{}'.format(column_names[-1])} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could also have a pointers
field being an array of pointers. we are already not jsonapi compliant anyway
I think one option would be to make this lookup be a set of utility function and then let the writer of the view decide how they want to do the mapping of the pg error to the api error. |
@sloria since you looked at this – is there an easy way to go from attribute name to data key, given a schema? |
I think it'd be attr_to_data_key = {
field.attribute or field_name: field.data_key or field_name
for field_name, field in schema.fields.items()
} |
cc @cancan101 @itajaja
This is obviously very rough and the final API will need to be more flexible, but the basic approach appears to work. Thoughts?