From 05442205bda3b603bbdb6f66ac1bac5546c59a45 Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Tue, 24 May 2022 14:56:10 -0700 Subject: [PATCH 1/4] make query context changes backwards compatible --- .../java/org/apache/druid/query/Query.java | 12 +- .../apache/druid/server/QueryLifecycle.java | 25 ++- .../apache/druid/server/QueryResource.java | 10 +- .../druid/server/QueryLifecycleTest.java | 202 ++++++++++++++++++ 4 files changed, 242 insertions(+), 7 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/Query.java b/processing/src/main/java/org/apache/druid/query/Query.java index f74327523a17..26d51e5ffbe2 100644 --- a/processing/src/main/java/org/apache/druid/query/Query.java +++ b/processing/src/main/java/org/apache/druid/query/Query.java @@ -64,6 +64,7 @@ }) public interface Query { + QueryContext IS_LEGACY_CONTEXT = new QueryContext(); String TIMESERIES = "timeseries"; String SEARCH = "search"; String TIME_BOUNDARY = "timeBoundary"; @@ -110,7 +111,16 @@ public interface Query * after it is deserialized. This is because {@link BaseQuery#getContext()} uses * {@link QueryContext#getMergedParams()} for serialization, and queries accept a map for deserialization. */ - QueryContext getQueryContext(); + default QueryContext getQueryContext() + { + return IS_LEGACY_CONTEXT; + } + + default boolean isLegacyContext() + { + //noinspection ObjectEquality + return getQueryContext() == Query.IS_LEGACY_CONTEXT; + } ContextType getContextValue(String key); diff --git a/server/src/main/java/org/apache/druid/server/QueryLifecycle.java b/server/src/main/java/org/apache/druid/server/QueryLifecycle.java index 8a38f7238e10..8b9aad4072ad 100644 --- a/server/src/main/java/org/apache/druid/server/QueryLifecycle.java +++ b/server/src/main/java/org/apache/druid/server/QueryLifecycle.java @@ -36,6 +36,7 @@ import org.apache.druid.query.DruidMetrics; import org.apache.druid.query.GenericQueryMetricsFactory; import org.apache.druid.query.Query; +import org.apache.druid.query.QueryContext; import org.apache.druid.query.QueryContexts; import org.apache.druid.query.QueryInterruptedException; import org.apache.druid.query.QueryMetrics; @@ -63,6 +64,7 @@ import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Set; import java.util.UUID; import java.util.concurrent.TimeUnit; @@ -186,11 +188,18 @@ public void initialize(final Query baseQuery) { transition(State.NEW, State.INITIALIZED); - baseQuery.getQueryContext().addDefaultParam(BaseQuery.QUERY_ID, UUID.randomUUID().toString()); - baseQuery.getQueryContext().addDefaultParams(defaultQueryConfig.getContext()); + if (baseQuery.isLegacyContext()) { + QueryContext context = new QueryContext(baseQuery.getContext()); + context.addDefaultParam(BaseQuery.QUERY_ID, UUID.randomUUID().toString()); + context.addDefaultParams(defaultQueryConfig.getContext()); - this.baseQuery = baseQuery; - this.toolChest = warehouse.getToolChest(baseQuery); + this.baseQuery = baseQuery.withOverriddenContext(context.getMergedParams()); + } else { + baseQuery.getQueryContext().addDefaultParam(BaseQuery.QUERY_ID, UUID.randomUUID().toString()); + baseQuery.getQueryContext().addDefaultParams(defaultQueryConfig.getContext()); + this.baseQuery = baseQuery; + } + this.toolChest = warehouse.getToolChest(this.baseQuery); } /** @@ -204,6 +213,12 @@ public void initialize(final Query baseQuery) public Access authorize(HttpServletRequest req) { transition(State.INITIALIZED, State.AUTHORIZING); + final Set contextKeys; + if (baseQuery.isLegacyContext()) { + contextKeys = baseQuery.getContext().keySet(); + } else { + contextKeys = baseQuery.getQueryContext().getUserParams().keySet(); + } final Iterable resourcesToAuthorize = Iterables.concat( Iterables.transform( baseQuery.getDataSource().getTableNames(), @@ -211,7 +226,7 @@ public Access authorize(HttpServletRequest req) ), authConfig.authorizeQueryContextParams() ? Iterables.transform( - baseQuery.getQueryContext().getUserParams().keySet(), + contextKeys, contextParam -> new ResourceAction(new Resource(contextParam, ResourceType.QUERY_CONTEXT), Action.WRITE) ) : Collections.emptyList() diff --git a/server/src/main/java/org/apache/druid/server/QueryResource.java b/server/src/main/java/org/apache/druid/server/QueryResource.java index a235fae199e2..93683465c476 100644 --- a/server/src/main/java/org/apache/druid/server/QueryResource.java +++ b/server/src/main/java/org/apache/druid/server/QueryResource.java @@ -46,6 +46,7 @@ import org.apache.druid.query.BadQueryException; import org.apache.druid.query.Query; import org.apache.druid.query.QueryCapacityExceededException; +import org.apache.druid.query.QueryContext; import org.apache.druid.query.QueryException; import org.apache.druid.query.QueryInterruptedException; import org.apache.druid.query.QueryTimeoutException; @@ -373,7 +374,14 @@ private Query readQuery( String prevEtag = getPreviousEtag(req); if (prevEtag != null) { - baseQuery.getQueryContext().addSystemParam(HEADER_IF_NONE_MATCH, prevEtag); + if (baseQuery.isLegacyContext()) { + QueryContext context = new QueryContext(baseQuery.getContext()); + context.addSystemParam(HEADER_IF_NONE_MATCH, prevEtag); + + return baseQuery.withOverriddenContext(context.getMergedParams()); + } else { + baseQuery.getQueryContext().addSystemParam(HEADER_IF_NONE_MATCH, prevEtag); + } } return baseQuery; diff --git a/server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java b/server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java index e49d3a87aeb3..a5c195d82591 100644 --- a/server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java @@ -21,19 +21,29 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Ordering; import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.Intervals; +import org.apache.druid.java.util.common.granularity.Granularities; +import org.apache.druid.java.util.common.granularity.Granularity; import org.apache.druid.java.util.common.guava.Sequences; import org.apache.druid.java.util.emitter.service.ServiceEmitter; +import org.apache.druid.query.BaseQuery; +import org.apache.druid.query.DataSource; import org.apache.druid.query.DefaultQueryConfig; import org.apache.druid.query.Druids; import org.apache.druid.query.GenericQueryMetricsFactory; +import org.apache.druid.query.NoopQueryRunner; +import org.apache.druid.query.Query; import org.apache.druid.query.QueryMetrics; import org.apache.druid.query.QueryRunner; import org.apache.druid.query.QuerySegmentWalker; import org.apache.druid.query.QueryToolChest; import org.apache.druid.query.QueryToolChestWarehouse; +import org.apache.druid.query.TableDataSource; import org.apache.druid.query.aggregation.CountAggregatorFactory; +import org.apache.druid.query.filter.DimFilter; +import org.apache.druid.query.spec.QuerySegmentSpec; import org.apache.druid.query.timeseries.TimeseriesQuery; import org.apache.druid.server.log.RequestLogger; import org.apache.druid.server.security.Access; @@ -45,6 +55,9 @@ import org.apache.druid.server.security.Resource; import org.apache.druid.server.security.ResourceType; import org.easymock.EasyMock; +import org.joda.time.DateTimeZone; +import org.joda.time.Duration; +import org.joda.time.Interval; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -52,7 +65,11 @@ import org.junit.Test; import org.junit.rules.ExpectedException; +import javax.annotation.Nullable; import javax.servlet.http.HttpServletRequest; +import java.util.Collections; +import java.util.List; +import java.util.Map; public class QueryLifecycleTest { @@ -244,6 +261,39 @@ public void testAuthorizeQueryContext_notAuthorized() Assert.assertFalse(lifecycle.authorize(mockRequest()).isAllowed()); } + @Test + public void testAuthorizeLegacyQueryContext_authorized() + { + EasyMock.expect(queryConfig.getContext()).andReturn(ImmutableMap.of()).anyTimes(); + EasyMock.expect(authConfig.authorizeQueryContextParams()).andReturn(true).anyTimes(); + EasyMock.expect(authenticationResult.getIdentity()).andReturn(IDENTITY).anyTimes(); + EasyMock.expect(authenticationResult.getAuthorizerName()).andReturn(AUTHORIZER).anyTimes(); + EasyMock.expect(authorizer.authorize(authenticationResult, new Resource("fake", ResourceType.DATASOURCE), Action.READ)) + .andReturn(Access.OK); + EasyMock.expect(authorizer.authorize(authenticationResult, new Resource("foo", ResourceType.QUERY_CONTEXT), Action.WRITE)) + .andReturn(Access.OK); + EasyMock.expect(authorizer.authorize(authenticationResult, new Resource("baz", ResourceType.QUERY_CONTEXT), Action.WRITE)).andReturn(Access.OK); + // to use legacy query context with context authorization, even system generated things like queryId need to be explicitly added + EasyMock.expect(authorizer.authorize(authenticationResult, new Resource("queryId", ResourceType.QUERY_CONTEXT), Action.WRITE)) + .andReturn(Access.OK); + + EasyMock.expect(toolChestWarehouse.getToolChest(EasyMock.anyObject())) + .andReturn(toolChest) + .once(); + + replayAll(); + + final LegacyContextQuery query = new LegacyContextQuery(ImmutableMap.of("foo", "bar", "baz", "qux")); + + lifecycle.initialize(query); + + Assert.assertTrue(lifecycle.getQuery().getContext().containsKey("foo")); + Assert.assertTrue(lifecycle.getQuery().getContext().containsKey("baz")); + Assert.assertTrue(lifecycle.getQuery().getContext().containsKey("queryId")); + + Assert.assertTrue(lifecycle.authorize(mockRequest()).isAllowed()); + } + private HttpServletRequest mockRequest() { HttpServletRequest request = EasyMock.createNiceMock(HttpServletRequest.class); @@ -274,4 +324,156 @@ private void replayAll() authorizer ); } + + private static class LegacyContextQuery implements Query + { + private final Map context; + + private LegacyContextQuery(Map context) + { + this.context = context; + } + + @Override + public DataSource getDataSource() + { + return new TableDataSource("fake"); + } + + @Override + public boolean hasFilters() + { + return false; + } + + @Override + public DimFilter getFilter() + { + return null; + } + + @Override + public String getType() + { + return "legacy-context-query"; + } + + @Override + public QueryRunner getRunner(QuerySegmentWalker walker) + { + return new NoopQueryRunner(); + } + + @Override + public List getIntervals() + { + return Collections.singletonList(Intervals.ETERNITY); + } + + @Override + public Duration getDuration() + { + return getIntervals().get(0).toDuration(); + } + + @Override + public Granularity getGranularity() + { + return Granularities.ALL; + } + + @Override + public DateTimeZone getTimezone() + { + return DateTimeZone.UTC; + } + + @Override + public Map getContext() + { + return context; + } + + @Override + public boolean getContextBoolean(String key, boolean defaultValue) + { + if (context == null || !context.containsKey(key)) { + return defaultValue; + } + return (boolean) context.get(key); + } + + @Override + public boolean isDescending() + { + return false; + } + + @Override + public Ordering getResultOrdering() + { + return Ordering.natural(); + } + + @Override + public Query withQuerySegmentSpec(QuerySegmentSpec spec) + { + return new LegacyContextQuery(context); + } + + @Override + public Query withId(String id) + { + context.put(BaseQuery.QUERY_ID, id); + return this; + } + + @Nullable + @Override + public String getId() + { + return (String) context.get(BaseQuery.QUERY_ID); + } + + @Override + public Query withSubQueryId(String subQueryId) + { + context.put(BaseQuery.SUB_QUERY_ID, subQueryId); + return this; + } + + @Nullable + @Override + public String getSubQueryId() + { + return (String) context.get(BaseQuery.SUB_QUERY_ID); + } + + @Override + public Query withDataSource(DataSource dataSource) + { + return this; + } + + @Override + public Query withOverriddenContext(Map contextOverride) + { + return new LegacyContextQuery(contextOverride); + } + + @Override + public Object getContextValue(String key, Object defaultValue) + { + if (!context.containsKey(key)) { + return defaultValue; + } + return context.get(key); + } + + @Override + public Object getContextValue(String key) + { + return context.get(key); + } + } } From d552fad8d7555afed8f4a57398997711cb2181fa Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Tue, 24 May 2022 15:19:45 -0700 Subject: [PATCH 2/4] javadocs --- .../main/java/org/apache/druid/query/Query.java | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/processing/src/main/java/org/apache/druid/query/Query.java b/processing/src/main/java/org/apache/druid/query/Query.java index 26d51e5ffbe2..dc2259492696 100644 --- a/processing/src/main/java/org/apache/druid/query/Query.java +++ b/processing/src/main/java/org/apache/druid/query/Query.java @@ -102,7 +102,13 @@ public interface Query Map getContext(); /** - * Returns QueryContext for this query. + * Returns QueryContext for this query. This type distinguishes between user provided, system default, and system + * generated query context keys so that authorization may be employed directly against the user supplied context + * values. + * + * Callers should first check {@link #isLegacyContext()} first to determine if the {@link Query} has implemented + * this method. If not, callers should fall back to using {@link #getContext()} and + * {@link #withOverriddenContext(Map)} as needed to manipulate the query. * * Note for query context serialization and deserialization. * Currently, once a query is serialized, its queryContext can be different from the original queryContext @@ -116,6 +122,14 @@ default QueryContext getQueryContext() return IS_LEGACY_CONTEXT; } + /** + * Checks if this {@link Query} implementation has implemented {@link #getQueryContext()}, which was added in + * Druid 0.23. If this method returns true, then callers should use {@link #getContext()} and + * {@link #withOverriddenContext(Map)} to manipulate the query context, and, if query context authorization is used + * (also added in 0.23), then permissions to system generated and system default context keys must also be added. + * It is recommended to not use query context authorization unless all queries implement {@link #getQueryContext()}. + */ + @Deprecated default boolean isLegacyContext() { //noinspection ObjectEquality From d5431e4f60316778a9892955aa6e9f00ca721b6b Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Tue, 24 May 2022 16:39:37 -0700 Subject: [PATCH 3/4] more test --- .../apache/druid/query/QueryContextTest.java | 187 ++++++++++++++++++ .../druid/server/QueryLifecycleTest.java | 172 +--------------- 2 files changed, 189 insertions(+), 170 deletions(-) diff --git a/processing/src/test/java/org/apache/druid/query/QueryContextTest.java b/processing/src/test/java/org/apache/druid/query/QueryContextTest.java index 3ff961b3f0d0..61609fa0336e 100644 --- a/processing/src/test/java/org/apache/druid/query/QueryContextTest.java +++ b/processing/src/test/java/org/apache/druid/query/QueryContextTest.java @@ -20,11 +20,26 @@ package org.apache.druid.query; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Ordering; import nl.jqno.equalsverifier.EqualsVerifier; import nl.jqno.equalsverifier.Warning; +import org.apache.druid.java.util.common.Intervals; +import org.apache.druid.java.util.common.granularity.Granularities; +import org.apache.druid.java.util.common.granularity.Granularity; +import org.apache.druid.query.aggregation.CountAggregatorFactory; +import org.apache.druid.query.filter.DimFilter; +import org.apache.druid.query.spec.QuerySegmentSpec; +import org.joda.time.DateTimeZone; +import org.joda.time.Duration; +import org.joda.time.Interval; import org.junit.Assert; import org.junit.Test; +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.List; +import java.util.Map; + public class QueryContextTest { @Test @@ -232,4 +247,176 @@ public void testGetMergedParams() Assert.assertSame(context.getMergedParams(), context.getMergedParams()); } + + @Test + public void testLegacyReturnsLegacy() + { + Query legacy = new LegacyContextQuery(ImmutableMap.of("foo", "bar")); + Assert.assertTrue(legacy.isLegacyContext()); + } + + @Test + public void testNonLegacyIsNotLegacyContext() + { + Query timeseries = Druids.newTimeseriesQueryBuilder() + .dataSource("test") + .intervals("2015-01-02/2015-01-03") + .granularity(Granularities.DAY) + .aggregators(Collections.singletonList(new CountAggregatorFactory("theCount"))) + .context(ImmutableMap.of("foo", "bar")) + .build(); + Assert.assertFalse(timeseries.isLegacyContext()); + } + + public static class LegacyContextQuery implements Query + { + private final Map context; + + public LegacyContextQuery(Map context) + { + this.context = context; + } + + @Override + public DataSource getDataSource() + { + return new TableDataSource("fake"); + } + + @Override + public boolean hasFilters() + { + return false; + } + + @Override + public DimFilter getFilter() + { + return null; + } + + @Override + public String getType() + { + return "legacy-context-query"; + } + + @Override + public QueryRunner getRunner(QuerySegmentWalker walker) + { + return new NoopQueryRunner(); + } + + @Override + public List getIntervals() + { + return Collections.singletonList(Intervals.ETERNITY); + } + + @Override + public Duration getDuration() + { + return getIntervals().get(0).toDuration(); + } + + @Override + public Granularity getGranularity() + { + return Granularities.ALL; + } + + @Override + public DateTimeZone getTimezone() + { + return DateTimeZone.UTC; + } + + @Override + public Map getContext() + { + return context; + } + + @Override + public boolean getContextBoolean(String key, boolean defaultValue) + { + if (context == null || !context.containsKey(key)) { + return defaultValue; + } + return (boolean) context.get(key); + } + + @Override + public boolean isDescending() + { + return false; + } + + @Override + public Ordering getResultOrdering() + { + return Ordering.natural(); + } + + @Override + public Query withQuerySegmentSpec(QuerySegmentSpec spec) + { + return new LegacyContextQuery(context); + } + + @Override + public Query withId(String id) + { + context.put(BaseQuery.QUERY_ID, id); + return this; + } + + @Nullable + @Override + public String getId() + { + return (String) context.get(BaseQuery.QUERY_ID); + } + + @Override + public Query withSubQueryId(String subQueryId) + { + context.put(BaseQuery.SUB_QUERY_ID, subQueryId); + return this; + } + + @Nullable + @Override + public String getSubQueryId() + { + return (String) context.get(BaseQuery.SUB_QUERY_ID); + } + + @Override + public Query withDataSource(DataSource dataSource) + { + return this; + } + + @Override + public Query withOverriddenContext(Map contextOverride) + { + return new LegacyContextQuery(contextOverride); + } + + @Override + public Object getContextValue(String key, Object defaultValue) + { + if (!context.containsKey(key)) { + return defaultValue; + } + return context.get(key); + } + + @Override + public Object getContextValue(String key) + { + return context.get(key); + } + } } diff --git a/server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java b/server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java index a5c195d82591..bfc612eaaa6a 100644 --- a/server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java @@ -21,29 +21,20 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Ordering; import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.Intervals; -import org.apache.druid.java.util.common.granularity.Granularities; -import org.apache.druid.java.util.common.granularity.Granularity; import org.apache.druid.java.util.common.guava.Sequences; import org.apache.druid.java.util.emitter.service.ServiceEmitter; -import org.apache.druid.query.BaseQuery; -import org.apache.druid.query.DataSource; import org.apache.druid.query.DefaultQueryConfig; import org.apache.druid.query.Druids; import org.apache.druid.query.GenericQueryMetricsFactory; -import org.apache.druid.query.NoopQueryRunner; -import org.apache.druid.query.Query; +import org.apache.druid.query.QueryContextTest; import org.apache.druid.query.QueryMetrics; import org.apache.druid.query.QueryRunner; import org.apache.druid.query.QuerySegmentWalker; import org.apache.druid.query.QueryToolChest; import org.apache.druid.query.QueryToolChestWarehouse; -import org.apache.druid.query.TableDataSource; import org.apache.druid.query.aggregation.CountAggregatorFactory; -import org.apache.druid.query.filter.DimFilter; -import org.apache.druid.query.spec.QuerySegmentSpec; import org.apache.druid.query.timeseries.TimeseriesQuery; import org.apache.druid.server.log.RequestLogger; import org.apache.druid.server.security.Access; @@ -55,9 +46,6 @@ import org.apache.druid.server.security.Resource; import org.apache.druid.server.security.ResourceType; import org.easymock.EasyMock; -import org.joda.time.DateTimeZone; -import org.joda.time.Duration; -import org.joda.time.Interval; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -65,11 +53,7 @@ import org.junit.Test; import org.junit.rules.ExpectedException; -import javax.annotation.Nullable; import javax.servlet.http.HttpServletRequest; -import java.util.Collections; -import java.util.List; -import java.util.Map; public class QueryLifecycleTest { @@ -283,7 +267,7 @@ public void testAuthorizeLegacyQueryContext_authorized() replayAll(); - final LegacyContextQuery query = new LegacyContextQuery(ImmutableMap.of("foo", "bar", "baz", "qux")); + final QueryContextTest.LegacyContextQuery query = new QueryContextTest.LegacyContextQuery(ImmutableMap.of("foo", "bar", "baz", "qux")); lifecycle.initialize(query); @@ -324,156 +308,4 @@ private void replayAll() authorizer ); } - - private static class LegacyContextQuery implements Query - { - private final Map context; - - private LegacyContextQuery(Map context) - { - this.context = context; - } - - @Override - public DataSource getDataSource() - { - return new TableDataSource("fake"); - } - - @Override - public boolean hasFilters() - { - return false; - } - - @Override - public DimFilter getFilter() - { - return null; - } - - @Override - public String getType() - { - return "legacy-context-query"; - } - - @Override - public QueryRunner getRunner(QuerySegmentWalker walker) - { - return new NoopQueryRunner(); - } - - @Override - public List getIntervals() - { - return Collections.singletonList(Intervals.ETERNITY); - } - - @Override - public Duration getDuration() - { - return getIntervals().get(0).toDuration(); - } - - @Override - public Granularity getGranularity() - { - return Granularities.ALL; - } - - @Override - public DateTimeZone getTimezone() - { - return DateTimeZone.UTC; - } - - @Override - public Map getContext() - { - return context; - } - - @Override - public boolean getContextBoolean(String key, boolean defaultValue) - { - if (context == null || !context.containsKey(key)) { - return defaultValue; - } - return (boolean) context.get(key); - } - - @Override - public boolean isDescending() - { - return false; - } - - @Override - public Ordering getResultOrdering() - { - return Ordering.natural(); - } - - @Override - public Query withQuerySegmentSpec(QuerySegmentSpec spec) - { - return new LegacyContextQuery(context); - } - - @Override - public Query withId(String id) - { - context.put(BaseQuery.QUERY_ID, id); - return this; - } - - @Nullable - @Override - public String getId() - { - return (String) context.get(BaseQuery.QUERY_ID); - } - - @Override - public Query withSubQueryId(String subQueryId) - { - context.put(BaseQuery.SUB_QUERY_ID, subQueryId); - return this; - } - - @Nullable - @Override - public String getSubQueryId() - { - return (String) context.get(BaseQuery.SUB_QUERY_ID); - } - - @Override - public Query withDataSource(DataSource dataSource) - { - return this; - } - - @Override - public Query withOverriddenContext(Map contextOverride) - { - return new LegacyContextQuery(contextOverride); - } - - @Override - public Object getContextValue(String key, Object defaultValue) - { - if (!context.containsKey(key)) { - return defaultValue; - } - return context.get(key); - } - - @Override - public Object getContextValue(String key) - { - return context.get(key); - } - } } From a0cc67fa8cb6680b74d833a3543a1489c9c2bdbb Mon Sep 17 00:00:00 2001 From: Clint Wylie Date: Tue, 24 May 2022 17:18:45 -0700 Subject: [PATCH 4/4] use null instead --- .../java/org/apache/druid/query/Query.java | 25 +++++-------------- .../apache/druid/query/QueryContextTest.java | 4 +-- .../apache/druid/server/QueryLifecycle.java | 4 +-- .../apache/druid/server/QueryResource.java | 2 +- .../druid/server/QueryLifecycleTest.java | 1 + 5 files changed, 12 insertions(+), 24 deletions(-) diff --git a/processing/src/main/java/org/apache/druid/query/Query.java b/processing/src/main/java/org/apache/druid/query/Query.java index dc2259492696..ced91a6383ca 100644 --- a/processing/src/main/java/org/apache/druid/query/Query.java +++ b/processing/src/main/java/org/apache/druid/query/Query.java @@ -64,7 +64,6 @@ }) public interface Query { - QueryContext IS_LEGACY_CONTEXT = new QueryContext(); String TIMESERIES = "timeseries"; String SEARCH = "search"; String TIME_BOUNDARY = "timeBoundary"; @@ -106,9 +105,10 @@ public interface Query * generated query context keys so that authorization may be employed directly against the user supplied context * values. * - * Callers should first check {@link #isLegacyContext()} first to determine if the {@link Query} has implemented - * this method. If not, callers should fall back to using {@link #getContext()} and - * {@link #withOverriddenContext(Map)} as needed to manipulate the query. + * This method is marked @Nullable, but is only so for backwards compatibility with Druid versions older than 0.23. + * Callers should check if the result of this method is null, and if so, they are dealing with a legacy query + * implementation, and should fall back to using {@link #getContext()} and {@link #withOverriddenContext(Map)} to + * manipulate the query context. * * Note for query context serialization and deserialization. * Currently, once a query is serialized, its queryContext can be different from the original queryContext @@ -117,23 +117,10 @@ public interface Query * after it is deserialized. This is because {@link BaseQuery#getContext()} uses * {@link QueryContext#getMergedParams()} for serialization, and queries accept a map for deserialization. */ + @Nullable default QueryContext getQueryContext() { - return IS_LEGACY_CONTEXT; - } - - /** - * Checks if this {@link Query} implementation has implemented {@link #getQueryContext()}, which was added in - * Druid 0.23. If this method returns true, then callers should use {@link #getContext()} and - * {@link #withOverriddenContext(Map)} to manipulate the query context, and, if query context authorization is used - * (also added in 0.23), then permissions to system generated and system default context keys must also be added. - * It is recommended to not use query context authorization unless all queries implement {@link #getQueryContext()}. - */ - @Deprecated - default boolean isLegacyContext() - { - //noinspection ObjectEquality - return getQueryContext() == Query.IS_LEGACY_CONTEXT; + return null; } ContextType getContextValue(String key); diff --git a/processing/src/test/java/org/apache/druid/query/QueryContextTest.java b/processing/src/test/java/org/apache/druid/query/QueryContextTest.java index 61609fa0336e..3654f85af175 100644 --- a/processing/src/test/java/org/apache/druid/query/QueryContextTest.java +++ b/processing/src/test/java/org/apache/druid/query/QueryContextTest.java @@ -252,7 +252,7 @@ public void testGetMergedParams() public void testLegacyReturnsLegacy() { Query legacy = new LegacyContextQuery(ImmutableMap.of("foo", "bar")); - Assert.assertTrue(legacy.isLegacyContext()); + Assert.assertNull(legacy.getQueryContext()); } @Test @@ -265,7 +265,7 @@ public void testNonLegacyIsNotLegacyContext() .aggregators(Collections.singletonList(new CountAggregatorFactory("theCount"))) .context(ImmutableMap.of("foo", "bar")) .build(); - Assert.assertFalse(timeseries.isLegacyContext()); + Assert.assertNotNull(timeseries.getQueryContext()); } public static class LegacyContextQuery implements Query diff --git a/server/src/main/java/org/apache/druid/server/QueryLifecycle.java b/server/src/main/java/org/apache/druid/server/QueryLifecycle.java index 8b9aad4072ad..ecada161cf62 100644 --- a/server/src/main/java/org/apache/druid/server/QueryLifecycle.java +++ b/server/src/main/java/org/apache/druid/server/QueryLifecycle.java @@ -188,7 +188,7 @@ public void initialize(final Query baseQuery) { transition(State.NEW, State.INITIALIZED); - if (baseQuery.isLegacyContext()) { + if (baseQuery.getQueryContext() == null) { QueryContext context = new QueryContext(baseQuery.getContext()); context.addDefaultParam(BaseQuery.QUERY_ID, UUID.randomUUID().toString()); context.addDefaultParams(defaultQueryConfig.getContext()); @@ -214,7 +214,7 @@ public Access authorize(HttpServletRequest req) { transition(State.INITIALIZED, State.AUTHORIZING); final Set contextKeys; - if (baseQuery.isLegacyContext()) { + if (baseQuery.getQueryContext() == null) { contextKeys = baseQuery.getContext().keySet(); } else { contextKeys = baseQuery.getQueryContext().getUserParams().keySet(); diff --git a/server/src/main/java/org/apache/druid/server/QueryResource.java b/server/src/main/java/org/apache/druid/server/QueryResource.java index 93683465c476..d71fdab56ca9 100644 --- a/server/src/main/java/org/apache/druid/server/QueryResource.java +++ b/server/src/main/java/org/apache/druid/server/QueryResource.java @@ -374,7 +374,7 @@ private Query readQuery( String prevEtag = getPreviousEtag(req); if (prevEtag != null) { - if (baseQuery.isLegacyContext()) { + if (baseQuery.getQueryContext() == null) { QueryContext context = new QueryContext(baseQuery.getContext()); context.addSystemParam(HEADER_IF_NONE_MATCH, prevEtag); diff --git a/server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java b/server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java index bfc612eaaa6a..1d1840b6c72e 100644 --- a/server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryLifecycleTest.java @@ -271,6 +271,7 @@ public void testAuthorizeLegacyQueryContext_authorized() lifecycle.initialize(query); + Assert.assertNull(lifecycle.getQuery().getQueryContext()); Assert.assertTrue(lifecycle.getQuery().getContext().containsKey("foo")); Assert.assertTrue(lifecycle.getQuery().getContext().containsKey("baz")); Assert.assertTrue(lifecycle.getQuery().getContext().containsKey("queryId"));