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

Fix: handle column default types correctly in SchemaCache #3750

Closed
wants to merge 3 commits into from

Conversation

mursidx
Copy link

@mursidx mursidx commented Oct 19, 2024

This pull request addresses issue #3706 by improving the handling of column default values in the SchemaCache.
The previous implementation did not correctly account for certain data types, which could lead to incorrect metadata retrieval.
This fix ensures that all relevant cases are covered, including scenarios where ad.adbin is not NULL.

Additionally, an entry has been added to the CHANGELOG.md for this fix.

Fixes #3706

Copy link
Member

@laurenceisla laurenceisla left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Here are my observations:

Comment on lines 628 to 629
WHEN ad.adbin IS NOT NULL THEN pg_get_expr(ad.adbin, ad.adrelid)::text
ELSE NULL -- Handle cases where none of the conditions are met
Copy link
Member

Choose a reason for hiding this comment

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

This still gives the same result since select pg_get_expr(null,null) returns null. What we need here is to get the column default before the domain default.

A test is needed to verify that changes work. Something similar to this one, but with another table with a column that overrides the domain default (like the one mentioned in the issue):

it "inserts a default on a DOMAIN with default" $
request methodPost "/evil_friends?columns=id,name" [("Prefer", "return=representation"), ("Prefer", "missing=default")]
[json| { "name": "Lu" } |]
`shouldRespondWith`
[json| [{"id": 666, "name": "Lu"}] |]
{ matchStatus = 201
, matchHeaders = ["Preference-Applied" <:> "missing=default, return=representation"]
}

CHANGELOG.md Outdated
Comment on lines 1100 to 1101
### Unreleased
- **fix**: Improved handling of column default values in `SchemaCache`. This addresses issue #3706 by ensuring all relevant cases are covered, including scenarios where `ad.adbin` is not NULL.
Copy link
Member

Choose a reason for hiding this comment

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

There is already an "Unreleased" section at the beginning of the document. The entry should go under the "Fixed" subtitle. Check the other entries for the format.

@mursidx
Copy link
Author

mursidx commented Oct 22, 2024

@laurenceisla ,
I have updated the SchemaCache.hs to prioritize column defaults as suggested. I also added a corresponding test case in InsertSpec.hs to verify the functionality and updated the CHANGELOG.md accordingly.

Comment on lines +7 to 9
### Fixed
- **fix**: Improved handling of column default values in `SchemaCache`. This addresses issue #3706 by ensuring all relevant cases are covered, including scenarios where `ad.adbin` is not NULL.

Copy link
Member

Choose a reason for hiding this comment

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

There is already a Fixed h3-section under the Unreleased h2-section. It should be added to that section. Also you are still not following the format as the other log entries below. Have a close look where the issue number is placed, note that the author is listed at the end, etc...

Comment on lines +566 to +575
it "inserts a default on a DOMAIN with default" $
request methodPost "/evil_friends?columns=id,name"
[("Prefer", "return=representation"), ("Prefer", "missing=default")]
[json| { "name": "Lu" } |]
`shouldRespondWith`
[json| [{"id": 666, "name": "Lu"}] |]
{ matchStatus = 201
, matchHeaders = ["Preference-Applied" <:> "missing=default, return=representation"]
}

Copy link
Member

Choose a reason for hiding this comment

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

Laurence asked you to write a test-case similar to this one, but with some changes (i.e. test for column default, not domain default)...

Instead of adjusting the test-case, so that it actually matches what you are trying to achieve.. you just copy-pasted the exact same thing, which is a duplicate of the test-case right after.

What are you trying to achieve?

@wolfgangwalther
Copy link
Member

Also note that all tests are failing, because your SQL is invalid + the style check is not passing either.

@mursidx mursidx closed this Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Doing a bulk insert with missing=default uses the default value of the domain instead of the column
3 participants