You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/02/09 13:17:50 UTC
[GitHub] [druid] abhishekagarwal87 opened a new pull request #10869: Allow for empty keys in hash map
abhishekagarwal87 opened a new pull request #10869:
URL: https://github.com/apache/druid/pull/10869
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
Fixes #10859.
There is also an alternate implementation as follows where we add following method to `GuiceAnnotationIntrospector` and remove `#findPropertyIgnorals`
```
@Override
public boolean hasIgnoreMarker(AnnotatedMember annotated)
{
return annotated.hasAnnotation(JacksonInject.class);
}
```
Though, it will throw an exception if the user tries to pass input with values for properties that were annotated with JacksonInject. As opposed to the current world, where such erroneous input is silently ignored.
### Description
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
#### Fixed the bug ...
#### Renamed the class ...
#### Added a forbidden-apis entry ...
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] 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](https://github.com/apache/druid/blob/master/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](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #10869: Allow for empty keys in hash map
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10869:
URL: https://github.com/apache/druid/pull/10869#discussion_r573489066
##########
File path: core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java
##########
@@ -101,13 +103,28 @@ public Object findInjectableValueId(AnnotatedMember m)
// delegate creators. I'm not 100% sure why it's not called, but guess it's because the argument
// is some Java type that Jackson already knows how to deserialize. Since there is only one argument,
// Jackson perhaps is able to just deserialize it without introspection.
+
if (ac instanceof AnnotatedParameter) {
final AnnotatedParameter ap = (AnnotatedParameter) ac;
if (ap.hasAnnotation(JsonProperty.class)) {
return JsonIgnoreProperties.Value.empty();
}
}
- return JsonIgnoreProperties.Value.forIgnoredProperties("");
+ // A map can have empty string keys e.g. https://github.com/apache/druid/issues/10859. By returning empty ignored
+ // list for map, we can allow for empty string keys in a map. A nested class within map
+ // can still be annotated with JacksonInject and still be non-deserializable from user input
+ // Refer to {@link com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.createMapDeserializer} for details
+ // on how the ignored list is passed to map deserializer
+ if (ac instanceof AnnotatedClass) {
+ final AnnotatedClass aClass = (AnnotatedClass) ac;
+ if (Map.class.isAssignableFrom(aClass.getAnnotated())) {
+ return JsonIgnoreProperties.Value.empty();
+ }
+ }
+
+ // We will allow serialization on empty properties. Properties marked with {@code @JacksonInject} are still
+ // not serialized if there is no getter marked with {@code @JsonProperty}
+ return JsonIgnoreProperties.Value.forIgnoredProperties("").withAllowGetters();
Review comment:
what is the `withAllowGetters` change for? I can't find any javadoc on this method
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10869: Allow for empty keys in hash map
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10869:
URL: https://github.com/apache/druid/pull/10869#discussion_r573463461
##########
File path: core/src/test/java/org/apache/druid/guice/DruidSecondaryModuleTest.java
##########
@@ -106,11 +192,30 @@ public ClassWithJacksonInject(
this.injected = injected;
}
- @JsonProperty
+ @JsonProperty("test")
Review comment:
It's not mandatory. Tests will pass even if I revert this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] Gabriel39 commented on a change in pull request #10869: Allow for empty keys in hash map
Posted by GitBox <gi...@apache.org>.
Gabriel39 commented on a change in pull request #10869:
URL: https://github.com/apache/druid/pull/10869#discussion_r573465146
##########
File path: core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java
##########
@@ -101,13 +103,28 @@ public Object findInjectableValueId(AnnotatedMember m)
// delegate creators. I'm not 100% sure why it's not called, but guess it's because the argument
// is some Java type that Jackson already knows how to deserialize. Since there is only one argument,
// Jackson perhaps is able to just deserialize it without introspection.
+
if (ac instanceof AnnotatedParameter) {
final AnnotatedParameter ap = (AnnotatedParameter) ac;
if (ap.hasAnnotation(JsonProperty.class)) {
return JsonIgnoreProperties.Value.empty();
}
}
- return JsonIgnoreProperties.Value.forIgnoredProperties("");
+ // A map can have empty string keys e.g. https://github.com/apache/druid/issues/10859. By returning empty ignored
+ // list for map, we can allow for empty string keys in a map. A nested class within map
+ // can still be annotated with JacksonInject and still be non-deserializable from user input
+ // Refer to {@link com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.createMapDeserializer} for details
+ // on how the ignored list is passed to map deserializer
+ if (ac instanceof AnnotatedClass) {
Review comment:
if we have a Map type field inside a class (just like Map is a field inside org.apache.druid.query.extraction.MapLookupExtractor), the bug reported by #10859 means when we deserialize org.apache.druid.query.extraction.MapLookupExtractor, the empty string key in Map will be ignored. So in this case, AnnotatedClass here is MapLookupExtractor instead Map.
To verify it, I remove this block and it works well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10869: Allow for empty keys in hash map
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10869:
URL: https://github.com/apache/druid/pull/10869#discussion_r573529209
##########
File path: core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java
##########
@@ -101,13 +103,28 @@ public Object findInjectableValueId(AnnotatedMember m)
// delegate creators. I'm not 100% sure why it's not called, but guess it's because the argument
// is some Java type that Jackson already knows how to deserialize. Since there is only one argument,
// Jackson perhaps is able to just deserialize it without introspection.
+
if (ac instanceof AnnotatedParameter) {
final AnnotatedParameter ap = (AnnotatedParameter) ac;
if (ap.hasAnnotation(JsonProperty.class)) {
return JsonIgnoreProperties.Value.empty();
}
}
- return JsonIgnoreProperties.Value.forIgnoredProperties("");
+ // A map can have empty string keys e.g. https://github.com/apache/druid/issues/10859. By returning empty ignored
+ // list for map, we can allow for empty string keys in a map. A nested class within map
+ // can still be annotated with JacksonInject and still be non-deserializable from user input
+ // Refer to {@link com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.createMapDeserializer} for details
+ // on how the ignored list is passed to map deserializer
+ if (ac instanceof AnnotatedClass) {
+ final AnnotatedClass aClass = (AnnotatedClass) ac;
+ if (Map.class.isAssignableFrom(aClass.getAnnotated())) {
+ return JsonIgnoreProperties.Value.empty();
+ }
+ }
+
+ // We will allow serialization on empty properties. Properties marked with {@code @JacksonInject} are still
+ // not serialized if there is no getter marked with {@code @JsonProperty}
+ return JsonIgnoreProperties.Value.forIgnoredProperties("").withAllowGetters();
Review comment:
This extra step is required because for serialization, Jackson tries to find `propertyIgnorals` on the getter method. Here is that annotated method from debugging a test
`[method org.apache.druid.guice.DruidSecondaryModuleTest$ConstructorWithJacksonInjectTest$ClassWithMapAndJacksonInject#getStringStringMap(0 params)]`
Our special check for `Map` won't cover this method and that will result in empty properties ignored during serialization.
We can either check if `ac` passed in `findPropertyIgnorals` is `AnnotatedMethod` and return type is `Map` and then return `Empty` or we can just allow serialization for empty properties. Latter works too and is less likely to cause bugs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10869: Allow for empty keys in hash map
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10869:
URL: https://github.com/apache/druid/pull/10869#discussion_r573469703
##########
File path: core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java
##########
@@ -101,13 +103,28 @@ public Object findInjectableValueId(AnnotatedMember m)
// delegate creators. I'm not 100% sure why it's not called, but guess it's because the argument
// is some Java type that Jackson already knows how to deserialize. Since there is only one argument,
// Jackson perhaps is able to just deserialize it without introspection.
+
if (ac instanceof AnnotatedParameter) {
final AnnotatedParameter ap = (AnnotatedParameter) ac;
if (ap.hasAnnotation(JsonProperty.class)) {
return JsonIgnoreProperties.Value.empty();
}
}
- return JsonIgnoreProperties.Value.forIgnoredProperties("");
+ // A map can have empty string keys e.g. https://github.com/apache/druid/issues/10859. By returning empty ignored
+ // list for map, we can allow for empty string keys in a map. A nested class within map
+ // can still be annotated with JacksonInject and still be non-deserializable from user input
+ // Refer to {@link com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.createMapDeserializer} for details
+ // on how the ignored list is passed to map deserializer
+ if (ac instanceof AnnotatedClass) {
Review comment:
@Gabriel39 the bug is that the empty string key in a `Map` is not deserialized. When you have a class that has a map as a field, this method will be called for both the outer class and the map. `MapLookupExtractionFnSerDeTest` is improved in this PR to test the case where a `MapLookupExtractor` has a map that has an empty string key. That test will fail without this block.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #10869: Allow for empty keys in hash map
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #10869:
URL: https://github.com/apache/druid/pull/10869
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10869: Allow for empty keys in hash map
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10869:
URL: https://github.com/apache/druid/pull/10869#discussion_r573525491
##########
File path: core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java
##########
@@ -101,13 +103,28 @@ public Object findInjectableValueId(AnnotatedMember m)
// delegate creators. I'm not 100% sure why it's not called, but guess it's because the argument
// is some Java type that Jackson already knows how to deserialize. Since there is only one argument,
// Jackson perhaps is able to just deserialize it without introspection.
+
if (ac instanceof AnnotatedParameter) {
final AnnotatedParameter ap = (AnnotatedParameter) ac;
if (ap.hasAnnotation(JsonProperty.class)) {
return JsonIgnoreProperties.Value.empty();
}
}
- return JsonIgnoreProperties.Value.forIgnoredProperties("");
+ // A map can have empty string keys e.g. https://github.com/apache/druid/issues/10859. By returning empty ignored
+ // list for map, we can allow for empty string keys in a map. A nested class within map
+ // can still be annotated with JacksonInject and still be non-deserializable from user input
+ // Refer to {@link com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.createMapDeserializer} for details
+ // on how the ignored list is passed to map deserializer
+ if (ac instanceof AnnotatedClass) {
+ final AnnotatedClass aClass = (AnnotatedClass) ac;
+ if (Map.class.isAssignableFrom(aClass.getAnnotated())) {
+ return JsonIgnoreProperties.Value.empty();
+ }
+ }
+
+ // We will allow serialization on empty properties. Properties marked with {@code @JacksonInject} are still
+ // not serialized if there is no getter marked with {@code @JsonProperty}
+ return JsonIgnoreProperties.Value.forIgnoredProperties("").withAllowGetters();
Review comment:
`JsonIgnoreProperties.Value` has following fields
```
protected final Set<String> _ignored;
protected final boolean _ignoreUnknown;
protected final boolean _allowGetters;
protected final boolean _allowSetters;
protected final boolean _merge
```
`_allowGetters` controls whether we will populate the field during serialization. By default, `_allowGetters` is set to false if using `forIgnoredProperties` but we override it to `true` using `withAllowGetters`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #10869: Allow for empty keys in hash map
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #10869:
URL: https://github.com/apache/druid/pull/10869#issuecomment-777260577
Thank you @Gabriel39 @jihoonson @clintropolis for the review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10869: Allow for empty keys in hash map
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10869:
URL: https://github.com/apache/druid/pull/10869#discussion_r573146707
##########
File path: core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java
##########
@@ -101,13 +103,28 @@ public Object findInjectableValueId(AnnotatedMember m)
// delegate creators. I'm not 100% sure why it's not called, but guess it's because the argument
// is some Java type that Jackson already knows how to deserialize. Since there is only one argument,
// Jackson perhaps is able to just deserialize it without introspection.
+
if (ac instanceof AnnotatedParameter) {
final AnnotatedParameter ap = (AnnotatedParameter) ac;
if (ap.hasAnnotation(JsonProperty.class)) {
return JsonIgnoreProperties.Value.empty();
}
}
- return JsonIgnoreProperties.Value.forIgnoredProperties("");
+ // A map can have empty string keys e.g. https://github.com/apache/druid/issues/10859. By returning empty ignored
+ // list for map, we can allow for empty string keys in a map. A nested class within map
+ // can still be annotated with JacksonInject and still be non-deserializable from user input
+ // Refer to {@link com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.createMapDeserializer} for details
+ // on how the ignored list is passed to map deserializer
+ if (ac instanceof AnnotatedClass) {
Review comment:
this block will ensure that we don't ignore string keys if the class is a map type.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #10869: Allow for empty keys in hash map
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10869:
URL: https://github.com/apache/druid/pull/10869#discussion_r573461294
##########
File path: core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java
##########
@@ -101,13 +103,28 @@ public Object findInjectableValueId(AnnotatedMember m)
// delegate creators. I'm not 100% sure why it's not called, but guess it's because the argument
// is some Java type that Jackson already knows how to deserialize. Since there is only one argument,
// Jackson perhaps is able to just deserialize it without introspection.
+
if (ac instanceof AnnotatedParameter) {
final AnnotatedParameter ap = (AnnotatedParameter) ac;
if (ap.hasAnnotation(JsonProperty.class)) {
return JsonIgnoreProperties.Value.empty();
}
}
- return JsonIgnoreProperties.Value.forIgnoredProperties("");
+ // A map can have empty string keys e.g. https://github.com/apache/druid/issues/10859. By returning empty ignored
+ // list for map, we can allow for empty string keys in a map. A nested class within map
+ // can still be annotated with JacksonInject and still be non-deserializable from user input
+ // Refer to {@link com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.createMapDeserializer} for details
+ // on how the ignored list is passed to map deserializer
+ if (ac instanceof AnnotatedClass) {
+ final AnnotatedClass aClass = (AnnotatedClass) ac;
+ if (Map.class.isAssignableFrom(aClass.getAnnotated())) {
+ return JsonIgnoreProperties.Value.empty();
+ }
+ }
+
+ // We will allow serialization on empty properties. Properties marked with {@code @JacksonInject} are still
+ // not serialized if there is no getter marked with {@code @JsonProperty}
Review comment:
There is none. which is why it's ok to allow getters otherwise we might have serialized fields marked with @JacksonInject.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] Gabriel39 commented on a change in pull request #10869: Allow for empty keys in hash map
Posted by GitBox <gi...@apache.org>.
Gabriel39 commented on a change in pull request #10869:
URL: https://github.com/apache/druid/pull/10869#discussion_r573127840
##########
File path: core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java
##########
@@ -101,13 +103,28 @@ public Object findInjectableValueId(AnnotatedMember m)
// delegate creators. I'm not 100% sure why it's not called, but guess it's because the argument
// is some Java type that Jackson already knows how to deserialize. Since there is only one argument,
// Jackson perhaps is able to just deserialize it without introspection.
+
if (ac instanceof AnnotatedParameter) {
final AnnotatedParameter ap = (AnnotatedParameter) ac;
if (ap.hasAnnotation(JsonProperty.class)) {
return JsonIgnoreProperties.Value.empty();
}
}
- return JsonIgnoreProperties.Value.forIgnoredProperties("");
+ // A map can have empty string keys e.g. https://github.com/apache/druid/issues/10859. By returning empty ignored
+ // list for map, we can allow for empty string keys in a map. A nested class within map
+ // can still be annotated with JacksonInject and still be non-deserializable from user input
+ // Refer to {@link com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.createMapDeserializer} for details
+ // on how the ignored list is passed to map deserializer
+ if (ac instanceof AnnotatedClass) {
Review comment:
I'm not sure whether this if-else is necessary. Actually, the bug reported by https://github.com/apache/druid/issues/10859 is caused by ignoring empty string keys in a map when deserialize org.apache.druid.query.extraction.MapLookupExtractor instead of Map
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10869: Allow for empty keys in hash map
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10869:
URL: https://github.com/apache/druid/pull/10869#discussion_r573428873
##########
File path: core/src/test/java/org/apache/druid/guice/DruidSecondaryModuleTest.java
##########
@@ -106,11 +192,30 @@ public ClassWithJacksonInject(
this.injected = injected;
}
- @JsonProperty
+ @JsonProperty("test")
Review comment:
Is this change mandatory? I hope it's not since we usually omit the property name in the annotation if the getter name implies it.
##########
File path: core/src/main/java/org/apache/druid/guice/GuiceAnnotationIntrospector.java
##########
@@ -101,13 +103,28 @@ public Object findInjectableValueId(AnnotatedMember m)
// delegate creators. I'm not 100% sure why it's not called, but guess it's because the argument
// is some Java type that Jackson already knows how to deserialize. Since there is only one argument,
// Jackson perhaps is able to just deserialize it without introspection.
+
if (ac instanceof AnnotatedParameter) {
final AnnotatedParameter ap = (AnnotatedParameter) ac;
if (ap.hasAnnotation(JsonProperty.class)) {
return JsonIgnoreProperties.Value.empty();
}
}
- return JsonIgnoreProperties.Value.forIgnoredProperties("");
+ // A map can have empty string keys e.g. https://github.com/apache/druid/issues/10859. By returning empty ignored
+ // list for map, we can allow for empty string keys in a map. A nested class within map
+ // can still be annotated with JacksonInject and still be non-deserializable from user input
+ // Refer to {@link com.fasterxml.jackson.databind.deser.BasicDeserializerFactory.createMapDeserializer} for details
+ // on how the ignored list is passed to map deserializer
+ if (ac instanceof AnnotatedClass) {
+ final AnnotatedClass aClass = (AnnotatedClass) ac;
+ if (Map.class.isAssignableFrom(aClass.getAnnotated())) {
+ return JsonIgnoreProperties.Value.empty();
+ }
+ }
+
+ // We will allow serialization on empty properties. Properties marked with {@code @JacksonInject} are still
+ // not serialized if there is no getter marked with {@code @JsonProperty}
Review comment:
Do we have any class that has a field injected with `@JacksonInject` and a getter method with `@JsonProperty` for that field? It seems pretty strange if we have any.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org