Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@JsonValue fails for Java Record #3063

Closed
cowwoc opened this issue Feb 21, 2021 · 15 comments
Closed

@JsonValue fails for Java Record #3063

cowwoc opened this issue Feb 21, 2021 · 15 comments
Labels
Record Issue related to JDK17 java.lang.Record support
Milestone

Comments

@cowwoc
Copy link

cowwoc commented Feb 21, 2021

Version information
2.12.1
OpenJDK 15.0.1

To Reproduce

Given:

public final record GetLocations(@JsonValue Map<String, URI> nameToLocation)
{
	@JsonCreator
	public GetLocations(Map<String, URI> nameToLocation)
	{
		assertThat(nameToLocation, "nameToLocation").isNotNull();
		this.nameToLocation = new HashMap<>(nameToLocation);
	}
}

I am expecting Jackson to serialize the Map to JSON but instead I get the following exception:

Problem with definition of [AnnotedClass GetLocations]: Multiple 'as-value' properties defined ([field GetLocations#nameToLocation] vs [method GetLocations#nameToLocation()])

@cowwoc cowwoc added the to-evaluate Issue that has been received but not yet evaluated label Feb 21, 2021
@cowwoc
Copy link
Author

cowwoc commented Feb 21, 2021

Adding the following code works around the problem (though ideologically, I'd prefer to serialize the return value of the getter than the field):

@JsonValue(false)
@Override
public Map<String, URI> nameToLocation()
{
	return nameToLocation;
}

@cowtowncoder
Copy link
Member

cowtowncoder commented Feb 21, 2021

Very likely related to the general issue (hoped to be tackled in 2.13) of matching @JsonCreator (constructor) parameters to fields, so that effectively @JsonValue would be invisible to property handling logic.

On plus side, if so, would get solved along with quite a few other problems.

@cowtowncoder cowtowncoder added 2.13 Record Issue related to JDK17 java.lang.Record support and removed to-evaluate Issue that has been received but not yet evaluated labels Feb 21, 2021
markelliot added a commit to markelliot/allezgo that referenced this issue Jul 1, 2021
@cowtowncoder cowtowncoder added 2.14 and removed 2.13 labels Jul 15, 2021
@pjfanning
Copy link
Member

@cowtowncoder if I read this issue correctly, then this appears to be similar to an issue in Scala. Java Records are similar to Scala Case Classes. In a Case Class, the constructor params are automatically accessible publicly - essentially Scala compiler creates a scala class with a field and a method with the name of the field. Annotations are added to the field unless you use a special form of the annotation (@(JsonValue @getter) is an annotation that is added to the derived getter method instead of the field). Scala 3 has a bug where the @getter is ignored so the annotation is not treated correctly.

So the ask on my part, is would it be possible to have jackson-databind PojoPropertiesCollector look for @JsonValue annotations on methods and fields?

@cowtowncoder
Copy link
Member

@pjfanning I think this would be part of the big rewrite of Creator-vs-Properties introspection that I was hoping to do for 2.13 but did not have time for. So got pushed forward.

Handling of @JsonValue should be synchronized in a way that avoids any issues with property detection, in case of serialization, yes. Where exactly that should be rectified I am not sure.

One thing that might be help would be reproducing this with regular @JsonCreator-using POJO instead of Record (assuming this is possible). Records have some special handling but this might not be due to that but more general issue.

@pjfanning
Copy link
Member

@cowtowncoder thanks for the update.

My comment above was really an enhancement request so that @JsonValue annotations could be supported on fields - would this be feasible?

@cowtowncoder
Copy link
Member

They are already, since Jackson 2.9 (see #1428).

@pjfanning
Copy link
Member

thanks @cowtowncoder - must be something else going on that is causing the scala 3 stuff not to work

@yihtserns
Copy link
Contributor

yihtserns commented Nov 9, 2022

...though ideologically, I'd prefer to serialize the return value of the getter than the field...

@cowwoc you can remove the @JsonValue annotation from the Record header, and annotate the accessor instead:

public final record GetLocations(Map<String, URI> nameToLocation)
{
	@JsonCreator
	public GetLocations(Map<String, URI> nameToLocation)
	{
		assertThat(nameToLocation, "nameToLocation").isNotNull();
		this.nameToLocation = new HashMap<>(nameToLocation);
	}

	@JsonValue
	@Override
	public Map<String, URI> nameToLocation()
	{
		return nameToLocation;
        }
}

A Trick

Since this issue is caused by (auto-)propagation of annotation on Records components, we learn that the decision to propagate the annotation to either field and/or accessor method is decided by the @Target supported by the annotation itself.

Since @JsonValue can be annotated on ElementType.FIELD & ElementType.METHOD, it gets propagated to both. Knowing this, you can create a custom meta-annotation for @JsonValue that targets only ElementType.METHOD:

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.METHOD)
@JacksonAnnotationsInside
@JsonValue
public @interface JsonValueAccessor {
}

Then this will then work:

public final record GetLocations(@JsonValueAccessor Map<String, URI> nameToLocation) // Your custom annotation
{
	@JsonCreator
	public GetLocations(Map<String, URI> nameToLocation)
	{
		assertThat(nameToLocation, "nameToLocation").isNotNull();
		this.nameToLocation = new HashMap<>(nameToLocation);
	}
}

@cowtowncoder cowtowncoder changed the title @JsonValue fails for Java Record @JsonValue fails for Java Record Jan 16, 2023
@cowtowncoder cowtowncoder added 2.15 Issues planned for 2.15 and removed 2.14 labels Jan 16, 2023
@cowtowncoder
Copy link
Member

Ok I think I know how this can be fixed quite easily: Method should have precedence over Fieldso let's use that as tie-breaker.

@cowtowncoder cowtowncoder added 2.14 and removed 2.15 Issues planned for 2.15 labels Jan 16, 2023
@cowtowncoder cowtowncoder added this to the 2.14.2 milestone Jan 16, 2023
@cowtowncoder
Copy link
Member

Fixed in 2.14 for 2.14.2.

@yihtserns
Copy link
Contributor

yihtserns commented Jan 16, 2023

@cowwoc please take note of the following gotcha for the workaround you used i.e.:

@JsonValue(false)
@Override
public Map<String, URI> nameToLocation()
{
	return nameToLocation;
}

The fix c23f772 for 2.14.2 can cope with that workaround, so you'll still get the same resulting JSON of {"first":"http://example.com","second":"http://google.com"} even if you don't remove it.

⚠️ BUT if and when you upgrade to 2.15.0, that workaround + the fix for #3352 (specifically #3737) will result in JSON of {"nameToLocation":{"first":"http://example.com","second":"http://google.com"}}, so you must remove it.

@cowwoc
Copy link
Author

cowwoc commented Jan 16, 2023

@yihtserns Got it. Thanks for the head's up.

@cowtowncoder
Copy link
Member

Hmmh. Ok that is bit unfortunate wrt compatibility, although not sure if anything can be done about it.
@JsonValue(false) means "do NOT use this method as 'JSON value' accessor.
But I guess had I realized there was a work-around that gets invalidated, I'd probably not have added this fix in 2.14 but only 2.15

@yihtserns
Copy link
Contributor

But I guess had I realized there was a work-around that gets invalidated, I'd probably not have added this fix in 2.14 but only 2.15

@cowtowncoder Eh your fix for 2.14.2 won't cause any problem with the workaround. Let me rephrase:

  1. The fix committed for this issue in 2.14.2 will NOT break the workaround.
    • Keeping/removing the workaround won't cause any problem.
  2. The fix committed for Do not require the usage of opens in a modular app when using records. #3352 in 2.15 WILL break the workaround.
    • Keeping the workaround will result in incorrect JSON so it must be removed.

Sorry for any confusion.

@cowtowncoder
Copy link
Member

@yihtserns ah yes. My bad; I did realize it after writing. Thank you for confirming.
That's a bit better, at least patch would not cause behavioral change.

dongjoon-hyun pushed a commit to apache/spark that referenced this issue Feb 7, 2023
### What changes were proposed in this pull request?
This pr aims upgrade `Jackson` related dependencies to 2.14.2

### Why are the changes needed?
This version include some bug fix and improvement of databind, such as:

- FasterXML/jackson-databind#3063
- FasterXML/jackson-databind#3699

The full release notes as follows:

- https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.14.2

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass Github Actions

Closes #39898 from LuciferYang/SPARK-42354.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
dongjoon-hyun pushed a commit to apache/spark that referenced this issue Feb 7, 2023
### What changes were proposed in this pull request?
This pr aims upgrade `Jackson` related dependencies to 2.14.2

### Why are the changes needed?
This version include some bug fix and improvement of databind, such as:

- FasterXML/jackson-databind#3063
- FasterXML/jackson-databind#3699

The full release notes as follows:

- https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.14.2

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass Github Actions

Closes #39898 from LuciferYang/SPARK-42354.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit eb8b97f)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
snmvaughan pushed a commit to snmvaughan/spark that referenced this issue Jun 20, 2023
### What changes were proposed in this pull request?
This pr aims upgrade `Jackson` related dependencies to 2.14.2

### Why are the changes needed?
This version include some bug fix and improvement of databind, such as:

- FasterXML/jackson-databind#3063
- FasterXML/jackson-databind#3699

The full release notes as follows:

- https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.14.2

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass Github Actions

Closes apache#39898 from LuciferYang/SPARK-42354.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit eb8b97f)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Record Issue related to JDK17 java.lang.Record support
Projects
None yet
Development

No branches or pull requests

4 participants