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

@JsonDeserialize(builder=...) not being honored by CSV #207

Open
airsupport opened this issue Jul 10, 2020 · 9 comments
Open

@JsonDeserialize(builder=...) not being honored by CSV #207

airsupport opened this issue Jul 10, 2020 · 9 comments
Labels

Comments

@airsupport
Copy link

Using Freebuilder (https://github.com/inferred/FreeBuilder) I build an interface of an object that I can serialize into Json. Using the JsonDeserialize tag, I can point to the inner Builder class to get the JsonProperty names, etc by tagging '@JsonDeserialize(builder =MyInterface.Builder.class)'.

This works fine when using Jackson to serialize into Json, but when using CSVSchema and CSVMapper, it claims that no @JsonProperty tags are present.

However, I can use a normal ObjectMapper and ObjectWriter to turn the object into a Json String just fine, so I'm not sure why the CSV stuff isn't working the same way.

Below is a link to a repo I created with an example. In lieu of using Freebuilder as a dependency, I committed the output class that FreeBuilder creates directly. This was requested by Tatu when I posted a question regarding this problem to the Google Group. Please feel free to use this repo for help with this issue.

https://github.com/airsupport/jsondeserialize-example

@cowtowncoder cowtowncoder changed the title JsonDeserialize tag not being honored by CSV @JsonDeserialize(builder=...) not being honored by CSV Jul 11, 2020
@cowtowncoder
Copy link
Member

Ok. As per exception, it looks like the problem is with CsvSchema introspection that does not work, not reading/writing in general. If schema was built like so:

          CsvSchema schema = CsvSchema.builder()
                  .setUseHeader(true)
                  .addColumn("foosInt")
                  .addColumn("foosString")
                  .addColumn("foosDouble")
                  .build();

code works as expected.

So the issue is that of making CsvSchema introspection also work with annotation as expected.

@airsupport
Copy link
Author

Shouldn't the schema build the header and generate the column names from the @JsonProperty tags automatically? If I add the @JsonProperty tags to the interface directly then the CsvSchema is generated perfectly fine. So I would expect that since @JsonDeserialize(builder =...) is pointing to the builder class, the CsvSchema would use those @JsonProperty tags to do the same thing.

@cowtowncoder
Copy link
Member

@airsupport Yes, it should ideally, I was just explaining that it does not do that currently, and that this is due to some missing interaction between property introspection and schema generation. Schema generation is not based on deserializer building, so lookups and redirections that deserializer construction and access make are probably not done for schema generation.
I will see what would it take to upgrade schema generation code to do necessary redirections.

@cowtowncoder
Copy link
Member

Ah, no. Looking bit deeper, the real problem is not linking, but rather that Foo itself does not declare any properties, so there is no way Schema constructor could find anything: it is actually Foo_Builder.Value that defines accessors. So to make this work would need to, for example, create schema like so:

CsvSchema schema = mapper.schemaFor(Foo_Builder.Value.class).withHeader();

since there is no way code could otherwise figure out way to go look for definitions; especially since schema is generated from serialization side (not deserializer) and builder information is not even gathered.

I don't know if there would be difference if schema generation was optionally allowed from deserialization side; I'll test this out next.

@airsupport
Copy link
Author

airsupport commented Jul 12, 2020

The problem that may be run into is I'm not sure if the Value class is accessible when built from FreeBuilder directly, as opposed to being put in the way I did it (I had to change some access modifiers to public). I will test that out as well.

I really appreciate all the help.

@cowtowncoder
Copy link
Member

cowtowncoder commented Jul 12, 2020

Ok, some more results: looks like it would be relatively easily to give an option to create schema from deserialization side; however, not sure that would help as it would still seems to introspect value type itself directly. So while I may want to add such an option eventually, this might not yet be the time.

One thing to note (which may or may not be obvious): construction of CsvSchema does not really have to be based on actual type being (de)serialized -- all it has to do is to match property names found from POJO with column indices of CSV to read/write.

@airsupport
Copy link
Author

airsupport commented Jul 13, 2020

I updated my example repo to be a little more accurate with the access modifiers as it would be when using FreeBuilder directly.

So, I understand what you're saying, and forgive my lack of understanding of the internals of Jackson. But wouldn't it make sense to generate the schema the same way Jackson generates the JSON? If it can get the JsonProperties from the Value class (which is private so unavailable for using it in the Schema builder directly) when doing normal Json work, how come it can't work that way here?

@cowtowncoder
Copy link
Member

@airsupport Good points on serializability as JSON: I am not sure what might be going on: while builder aspect is not found, value class should have info on properties to serialize.
I would need to dig deeper here, and hope to do that at some point -- unfortunately I have a bit long list of things to work on right now. But I have not forgotten this issue.

If you have time and interest, one thing you could try would be to locally modify CsvMapper method _addSchemaProperties() to use getDeserializationConfig().introspect(...) (instead of getSerializationConfig()) and see if that makes a difference.

@cowtowncoder cowtowncoder added the to-evaluate Issue that has been received but not yet evaluated label Aug 19, 2020
@cowtowncoder cowtowncoder removed the to-evaluate Issue that has been received but not yet evaluated label Oct 2, 2020
@airsupport
Copy link
Author

I appreciate you keeping this on your radar. Yeah, I also got busy and this fell of my radar and I forgot about it for a while. But anyway, I tried your recommendation and it didn't change anything. I also updated my dependencies in my example code to use the most up to date jackson-dataformat-csv.

@cowtowncoder cowtowncoder removed the 2.12 label Apr 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants