make query context changes backwards compatible#12564
Merged
abhishekagarwal87 merged 4 commits intoapache:masterfrom May 25, 2022
Merged
make query context changes backwards compatible#12564abhishekagarwal87 merged 4 commits intoapache:masterfrom
abhishekagarwal87 merged 4 commits intoapache:masterfrom
Conversation
6 tasks
| QueryContext getQueryContext(); | ||
| default QueryContext getQueryContext() | ||
| { | ||
| return IS_LEGACY_CONTEXT; |
Contributor
There was a problem hiding this comment.
From looking at the code in this PR, why not return null and do null tests?
Member
Author
There was a problem hiding this comment.
heh, I just left a comment about the same thing here #12396 (comment)
I considered this, but decided against it because it didn't seem like actual implementations should legitimately return null and so seemed strange to mark @Nullable. Im not very attached to this though, so I could do the null thing instead, and mark it as @Nullable and update the javadocs to note that its not actually nullable and is only for backwards compatibility and explain the implications.
Member
Author
There was a problem hiding this comment.
which since you're the 2nd person to suggest it, couple with the fact that i initially considered doing it that way, maybe it would be better?
jihoonson
approved these changes
May 25, 2022
imply-cheddar
approved these changes
May 25, 2022
abhishekagarwal87
pushed a commit
to abhishekagarwal87/druid
that referenced
this pull request
May 25, 2022
Adds a default implementation of getQueryContext, which was added to the Query interface in apache#12396. Query is marked with @ExtensionPoint, and lately we have been trying to be less volatile on these interfaces by providing default implementations to be more chill for extension writers. The way this default implementation is done in this PR is a bit strange due to the way that getQueryContext is used (mutated with system default and system generated keys); the default implementation has a specific object that it returns, and I added another temporary default method isLegacyContext that checks if the getQueryContext returns that object or not. If not, callers fall back to using getContext and withOverriddenContext to set these default and system values. I am open to other ideas as well, but this way should work at least without exploding, and added some tests to ensure that it is wired up correctly for QueryLifecycle, including the context authorization stuff. The added test shows the strange behavior if query context authorization is enabled, mainly that the system default and system generated query context keys also need to be granted as permissions for things to function correctly. This is not great, so I mentioned it in the javadocs as well. Not sure if it needs to be called out anywhere else.
abhishekagarwal87
added a commit
that referenced
this pull request
Jun 2, 2022
Adds a default implementation of getQueryContext, which was added to the Query interface in #12396. Query is marked with @ExtensionPoint, and lately we have been trying to be less volatile on these interfaces by providing default implementations to be more chill for extension writers. The way this default implementation is done in this PR is a bit strange due to the way that getQueryContext is used (mutated with system default and system generated keys); the default implementation has a specific object that it returns, and I added another temporary default method isLegacyContext that checks if the getQueryContext returns that object or not. If not, callers fall back to using getContext and withOverriddenContext to set these default and system values. I am open to other ideas as well, but this way should work at least without exploding, and added some tests to ensure that it is wired up correctly for QueryLifecycle, including the context authorization stuff. The added test shows the strange behavior if query context authorization is enabled, mainly that the system default and system generated query context keys also need to be granted as permissions for things to function correctly. This is not great, so I mentioned it in the javadocs as well. Not sure if it needs to be called out anywhere else. Co-authored-by: Clint Wylie <cwylie@apache.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a default implementation of
getQueryContext, which was added to theQueryinterface in #12396.Queryis marked with@ExtensionPoint, and lately we have been trying to be less volatile on these interfaces by providing default implementations to be more chill for extension writers.The way this default implementation is done in this PR is a bit strange due to the way that
getQueryContextis used (mutated with system default and system generated keys); the default implementation has a specific object that it returns, and I added another temporary default methodisLegacyContextthat checks if thegetQueryContextreturns that object or not. If not, callers fall back to usinggetContextandwithOverriddenContextto set these default and system values.I am open to other ideas as well, but this way should work at least without exploding, and added some tests to ensure that it is wired up correctly for
QueryLifecycle, including the context authorization stuff.The added test shows the strange behavior if query context authorization is enabled, mainly that the system default and system generated query context keys also need to be granted as permissions for things to function correctly. This is not great, so I mentioned it in the javadocs as well. Not sure if it needs to be called out anywhere else.