-
Notifications
You must be signed in to change notification settings - Fork 2.8k
server: cache default values (fix #3773) #4620
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
Conversation
|
Review app for commit 1a8dcb3 deployed to Heroku: https://hge-ci-pull-4620.herokuapp.com |
1a8dcb3 to
080cbc1
Compare
|
Review app for commit 080cbc1 deployed to Heroku: https://hge-ci-pull-4620.herokuapp.com |
080cbc1 to
73d81a7
Compare
|
Review app for commit ec713c6 deployed to Heroku: https://hge-ci-pull-4620.herokuapp.com |
8e9071e to
c7bcc68
Compare
|
Review app for commit c7bcc68 deployed to Heroku: https://hge-ci-pull-4620.herokuapp.com |
| data ReusableVariableInfo | ||
| = ReusableVariableInfo RQL.PGColumnType (ReusableDefaultValue PGScalarValue) | ||
| deriving (Show, Eq) | ||
| $(J.deriveToJSON J.defaultOptions{J.constructorTagModifier = drop 8} ''ReusableVariableInfo) |
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.
Why do we need the extra ReusableDefaultValue type? Isn't this equally readable without losing any type safety?
data ReusableVariableInfo
= ReusableVariableInfo
{ _rviType :: RQL.PGColumnType,
, _rviDefault :: Maybe PGScalarValue
} deriving (Show, Eq)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.
Because Maybe is subject a sort of Boolean blindness. In the current situation, we don't clearly separate AnnInpVals originating from the query, and those originating from user-provided arguments. So we have to string the default values through our code, and this is error-prone, as this issue shows. The reason that we incorrectly cached these kinds of queries in the first place is that the provided value was simply <|>'d with the default value (in validateVariables), thus forgetting the default value. ResuableDefaultValue forces us to think explicitly about what to do with default vs user-provided values, at least in some places.
Put differently: suppose you get a Maybe PGScalarValue somewhere, what kind of value is this? Is this a user-provided argument? If so, has the default value been applied? If not, where would you get that from? ReusableDefaultValue goes one step towards clarifying the answers to these questions.
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.
Right, I see what you mean. In that case wouldn't the standard pattern be a newtype wrapper on PGScalarValue like this?
newtype ReusableDefaultValue = ReusableDefaultValue PGScalarValue
data ReusableVariableInfo
= ReusableVariableInfo
{ _rviType :: RQL.PGColumnType,
, _rviDefault :: Maybe ReusableDefaultValue
} deriving (Show, Eq)i.e, why parameterize ReusableDefaultValue and why encompass Maybe too?
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.
With the newtype variant you now have to unpack two constructors to get to the value, as in:
f : Maybe ReusableDefaultValue -> a
f (Just (ReusableDefaultValue pgScalarValue)) = _
f Nothing = _So since data types are really cheap in Haskell, we might as well re-define Maybe to get a more convenient API:
f : ReusableDefaultValue -> a
f (ReusableDefault pgScalarValue) = _
f ReusableNoDefault = _Note that the two patterns directly give a lot of information about why we are doing pattern matching, in contrast with the newtype variant, where it's not quite clear, visually, what kind of value we are deconstructing. Indeed, in the second variant, the type signature is pretty much unnecessary to understand the code, whereas in the first variant one would need to be quite familiar with the codebase in order to be able to do without.
Anyway, this is not a critical point, so I'm happy to make this into a newtype if that allows this PR to get merged without further debate.
|
Review app for commit 4035613 deployed to Heroku: https://hge-ci-pull-4620.herokuapp.com |
|
Review app for commit 846be42 deployed to Heroku: https://hge-ci-pull-4620.herokuapp.com |
|
Review app for commit aedded7 deployed to Heroku: https://hge-ci-pull-4620.herokuapp.com |
aedded7 to
83e62bd
Compare
|
Review app for commit 83e62bd deployed to Heroku: https://hge-ci-pull-4620.herokuapp.com |
|
This now has a regression test, conform the discussion in #4661. |
| data AnnInpVal | ||
| = AnnInpVal | ||
| { _aivType :: !G.GType | ||
| , _aivVariable :: !(Maybe G.Variable) |
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.
Let's change G.Variable to VariableInfo where VariableInfo represents (G.Variable, Maybe AnnGValue).
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.
Currently, we seem to be adding this value here, after this change we don't need to do that anymore, validateVariables itself can build this VariableInfo and add it to the AnnInpVal.
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.
Your link doesn't work for me, unfortunately, so I am not sure what you are suggesting.
|
Left a few comments otherwise the overall idea looks good. I see a lot of friction because of how |
|
Review app for commit 21dba0e deployed to Heroku: https://hge-ci-pull-4620.herokuapp.com |
tirumaraiselvan
left a comment
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.
changelog
|
Not relevant any more. |
Description
This is a really dirty way to fix #3773 and fix #4577.
Changelog
CHANGELOG.mdis updated with user-facing content relevant to this PR.Affected components
Solution and Design
The origin of #3773 is that the "validation" stage not just validates correctness of input data, but also gives back updated values. In particular, the "validation" combines the GraphQL query specification with the argument values, so that any default values are overwritten. As a consequence, the default value was not accessible during the query planning stage.
As a dirty hack to this, this code adds a field to
AnnInpValthat specifies a default value (if any). This can then be updated at a later stage, so that the default value is available during the query planning stage.The real solution here would be:
Steps to test and verify
See #3773 and #4577.
Limitations, known bugs & workarounds
This PR does NOT contain a regression test, as I do not know of an easy way to test this. So when #4111 is merged, we should pay attention that this bug does not re-appear.
Server checklist
Catalog upgrade
Does this PR change Hasura Catalog version?
Metadata
Does this PR add a new Metadata feature?
GraphQL
Breaking changes