Add a `StrictTypeIdResolver` which would throw an exception if `type` is not in the predefined json subtype list. Use it in `DimensionSchema`. by cecemei · Pull Request #18565 · apache/druid

cecemei · 2025-09-23T18:24:07Z

Description

Add a StrictTypeIdResolver which would throw an exception if type is not in the predefined json subtype list. Use it in DimensionSchema.

Key changed/added classes in this PR

StrictTypeIdResolver
DimensionSchema
DimensionSchemaTest

Release note:

At ingestion time, dimension schemas are now strictly validated against allowed types. Previously an invalid type would fall back to string dimension, now such values are rejected. Users must specify type as one of the allowed types. Omitting type still defaults to string, preserving backward compatibility.

This PR has:

Copilot

Pull Request Overview

This PR adds strict type validation for JSON deserialization of DimensionSchema by introducing a custom StrictTypeIdResolver that throws exceptions for unknown types while preserving default behavior for missing types.

Implemented StrictTypeIdResolver to enforce strict validation of JSON type discriminators
Modified DimensionSchema to use the new strict type resolver instead of default Jackson behavior
Added comprehensive tests for both invalid type handling and default type fallback scenarios

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
StrictTypeIdResolver.java	New custom type resolver that validates type IDs against registered subtypes
DimensionSchema.java	Updated to use StrictTypeIdResolver with enhanced documentation
DimensionSchemaTest.java	Added tests for strict type validation and default type behavior
StringDimensionSchemaTest.java	Removed explicit type from test JSON to verify default behavior

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-25T15:18:49Z

+  private Map<String, Class<?>> typeMap;
+  private JavaType baseType;


The typeMap field is not thread-safe and could cause issues in concurrent environments. Consider using ConcurrentHashMap or making the field volatile with proper synchronization in ensureTypeMap().

hmmm, do we actually need to look into thread safety requirements here? I'm unfamiliar with this exact area but this comment plus a quick query of an ai agent does make me wonder if thread safety is necessary.

actually rewrite the pr to use a builder class, PTAL!

capistrant

one nit for you to consider if you you want to change

requested copilot review as well. I don't think either is legit blocker

I want us to double check on any thread safety requirements with the new custom @JsonTypeIdResolver

abhishekrb19 · 2025-10-01T00:27:54Z

Would this break existing specs that were persisted with an invalid type specified in the dimension schema? Or would those specs already default to string, so it wouldn’t be an issue? (I'm not sure when the jackson default will apply)

Copilot

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-03T21:48:03Z

+
+  StrictTypeIdResolver()
+  {
+    // Required default constructor for Jackson, the instance is never used


The comment states 'the instance is never used' but this may be misleading. Jackson may instantiate this constructor for reflection purposes. Consider clarifying that this constructor creates an unusable instance that should not be used for actual resolution.

Suggested change

// Required default constructor for Jackson, the instance is never used

// Required default constructor for Jackson's reflective instantiation.

// This creates an unusable instance that should not be used for actual resolution.

cecemei · 2025-10-03T23:15:36Z

Would this break existing specs that were persisted with an invalid type specified in the dimension schema? Or would those specs already default to string, so it wouldn’t be an issue? (I'm not sure when the jackson default will apply)

If the type is not set, it'd default to string, but if it's set a value that's not specified in JsonSubTypes, it'd throw an error, but we can add those to map to string too. Is there any existing specs that uses invalid types?

capistrant

@cecemei this is all just ingestion spec related, correct? So if users had been submitting specs in the past with invalid types, it would quietly default to string. But with your new behavior they will get an error? So to a user, this appears to be a breaking change, but really we are just fixing undesirable behavior? I think a release note section in the PR description should be created to specify how exactly the behavior is changing and what a user should do about it to fix (point them to the valid types?).

…nsionSchemaTest.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

cecemei · 2025-10-06T23:15:39Z

@cecemei this is all just ingestion spec related, correct? So if users had been submitting specs in the past with invalid types, it would quietly default to string. But with your new behavior they will get an error? So to a user, this appears to be a breaking change, but really we are just fixing undesirable behavior? I think a release note section in the PR description should be created to specify how exactly the behavior is changing and what a user should do about it to fix (point them to the valid types?).

True, added a release entry.

cecemei added 4 commits September 23, 2025 11:23

strict-type-id

e0294e2

test

5ab276a

Merge branch 'master' into serde

341b26e

trigger ci / empty commit

baf7813

cecemei changed the title ~~strict-type-id~~ Add a StrictTypeIdResolver which would throw an exception if type is not in the predefined json subtype list. Use it in DimensionSchema. Sep 24, 2025

cecemei marked this pull request as ready for review September 24, 2025 16:05

capistrant requested review from capistrant and Copilot September 25, 2025 15:17

Copilot AI reviewed Sep 25, 2025

View reviewed changes

capistrant previously approved these changes Sep 25, 2025

View reviewed changes

Comment thread processing/src/main/java/org/apache/druid/jackson/StrictTypeIdResolver.java Outdated

capistrant self-requested a review September 25, 2025 18:08

cecemei added 3 commits October 3, 2025 13:35

use-delegate

a73daeb

Merge branch 'master' into serde

d31dc50

unnecessary-null-check

f167cd4

cecemei requested a review from Copilot October 3, 2025 21:47

Copilot AI reviewed Oct 3, 2025

View reviewed changes

capistrant reviewed Oct 6, 2025

View reviewed changes

Comment thread processing/src/test/java/org/apache/druid/data/input/impl/DimensionSchemaTest.java Outdated

capistrant added the Release Notes label Oct 6, 2025

Update processing/src/test/java/org/apache/druid/data/input/impl/Dime…

71a2897

…nsionSchemaTest.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

capistrant approved these changes Oct 7, 2025

View reviewed changes

cecemei merged commit 94a1045 into apache:master Oct 7, 2025
109 of 110 checks passed

cecemei added this to the 35.0.0 milestone Oct 21, 2025

		private Map<String, Class<?>> typeMap;
		private JavaType baseType;

	// Required default constructor for Jackson, the instance is never used
	// Required default constructor for Jackson's reflective instantiation.
	// This creates an unusable instance that should not be used for actual resolution.

Conversation

cecemei commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key changed/added classes in this PR

Release note:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

capistrant Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

cecemei Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

capistrant left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abhishekrb19 commented Oct 1, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cecemei commented Oct 3, 2025

Uh oh!

capistrant left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cecemei commented Oct 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cecemei commented Sep 23, 2025 •

edited

Loading

capistrant left a comment •

edited

Loading