Skip to content

refactor: Make QueryBlocklistRule an interface for extensibility#19457

Open
abhishekrb19 wants to merge 2 commits into
apache:masterfrom
abhishekrb19:query_block_interface
Open

refactor: Make QueryBlocklistRule an interface for extensibility#19457
abhishekrb19 wants to merge 2 commits into
apache:masterfrom
abhishekrb19:query_block_interface

Conversation

@abhishekrb19
Copy link
Copy Markdown
Contributor

Summary

  • Converts QueryBlocklistRule from a concrete class to an interface with two methods: getRuleName() and matches(Query<?>).
  • Moves the existing implementation to DefaultQueryBlocklistRule with a default impl on the interface @JsonTypeInfo(defaultImpl = DefaultQueryBlocklistRule.class) so existing JSON configs without a "type" field continue to deserialize correctly.
  • Extensions can register new rule types as Jackson subtypes via SimpleModule.registerSubtypes(...) and we can even introduce new union rule types with different logic w/o polluting existing default implementation.

Motivation

The current blocklist logic is fixed to datasource/queryType/context matching. Making it an interface allows adding new rule types (e.g., regex-based, time-window-based, user-based) without modifying the existing code.

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@abhishekrb19 abhishekrb19 requested a review from jtuglu1 May 12, 2026 20:05
Copy link
Copy Markdown
Member

@FrankChen021 FrankChen021 left a comment

Choose a reason for hiding this comment

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

Severity Findings
P0 0
P1 0
P2 1
P3 0
Total 1

Reviewed 7 of 7 changed files.


This is an automated review by Codex GPT-5.5

* {@link org.apache.druid.server.broker.BrokerDynamicConfig} can detect changes correctly.
*/
public class QueryBlocklistRule
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type", defaultImpl = DefaultQueryBlocklistRule.class)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Fail unknown blocklist rule types instead of defaulting

Using defaultImpl = DefaultQueryBlocklistRule.class makes Jackson use the default rule not only when type is absent, but also when a non-empty type id cannot be resolved. For the new extension point, a broker or coordinator that lacks a registered custom rule subtype can silently deserialize that rule as DefaultQueryBlocklistRule; with Druid's unknown-property-tolerant mapper, extension-specific fields are ignored and any standard fields such as dataSources can turn into a broader block than intended. Please distinguish the legacy missing-type case from an unrecognized explicit type, so unknown extension rules fail fast rather than being interpreted as default rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants