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

DefaultRecordMapper cannot fill nested objects properties via constructors #9000

Closed
eduramiba opened this issue Jul 30, 2019 · 10 comments
Closed

Comments

@eduramiba
Copy link

eduramiba commented Jul 30, 2019

Expected behavior and actual behavior:

DefaultRecordMapper should be able to fill nested objects properties with methods such as fetchOneInto(X.class) using class and nested classes constructors properly annotated with @ConstructorProperties.

It will probably work using setters but I want all my models to be immutable and directly mapped by JooQ.

Steps to reproduce the problem

See MCVE here: https://github.com/eduramiba/jooq-issue-recordmapper

Following conversation at https://twitter.com/eduramiba/status/1149303730333986817

Versions:

  • jOOQ: 3.11.11

These I don't think they matter:

  • Java: 12 OpenJDK
  • Database (include vendor): PostgreSQL 11
  • OS: Ubuntu 18.04
  • JDBC Driver (include name if inofficial driver): PostgreSQL 42.2.6

Thank you!

@knutwannheden
Copy link
Contributor

Thank you for your report.

We will look into this and try to address it as part of the upcoming 3.12 release.

@eduramiba
Copy link
Author

Thanks!

@knutwannheden
Copy link
Contributor

I looked into the code and the DefaultRecordMapper can currently only handle nested mappings for "mutable" POJOs (i.e. where the POJO type being constructed (interface or class) declares matching fields or setters). For "immutable" POJOs (constructed via non-default constructor invocation), nested mappings have not yet been implemented.

Further I noticed that in your MCVE you are using the DefaultRecordMapper to construct an instance of ThingModel.Builder rather than ThingModel directly. Was the intention that doTestOne() should have called ResultQuery#fetchOneInto(Class) to have the DefaultRecordMapper directly create the ThingModel POJO?

@eduramiba
Copy link
Author

@knutwannheden Oops true, first test should be fetchOneInto(ThingModel.class) but it was copy-pasted. I change it now.

Using a builder is a workaround to getting 1 nesting level, but not more.

@knutwannheden
Copy link
Contributor

I think that we may have initially misunderstood your request on Twitter. According to the documentation of DefaultRecordMapper it is quite clear that the nested mapping only works when the POJOs are instantiated by default constructor. So I changed this issue into an enhancement / feature request.

For @ConstructorProperties annotated constructors the current logic isn't really suitable, as the object graph would have to be instantiated bottom-up rather than top-down.

Given the complexity of this issue, I am doubtful whether we will be able to fit it into the 3.12 release anymore.

I think we might however be able to change the DefaultRecordMapper so that it uses Configuration#recordMapperProvider() to create a new RecordMapper for the nested mappings. With the Builder classes you have, this should allow you to use a mapper for AuditInfo which first creates an AuditInfo.Builder object.

@eduramiba
Copy link
Author

I see, thank you for the suggestion. Is there some sample code on how to implement the special record mapper that would use the AuditInfo.Builder?

I already use some mappers for UDTs implementing RecordMapperProvider but not like this.

@knutwannheden
Copy link
Contributor

I will still have to test it myself to make sure it actually works, but I was thinking a RecordMapper like your lambda r -> r.into(ThingModel.Builder.class).build() should do the trick.

@knutwannheden
Copy link
Contributor

knutwannheden commented Jul 31, 2019

AFAICT this is really a duplicate of #6598

I will leave this issue open for now, however, so that we can verify the implementation against your excellent MCVE. Thanks again for the effort!

@knutwannheden
Copy link
Contributor

@eduramiba I have now committed a change to jOOQ 3.12 so that the DefaultRecordMapper internally also uses the RecordMapperProvider (see 2825bbd). You could leverage this to allow the DefaultRecordMapper to create your nested immutable POJOs through the corresponding Builder classes, if you really want to. But it is a bit ugly and with #6598 you would then no longer need this workaround.

The code to set up the Configuration would look something like this:

final Configuration configuration = create.configuration();
configuration.set(new RecordMapperProvider() {
    @SuppressWarnings("unchecked")
    @Override
    public <R extends Record, E> RecordMapper<R, E> provide(RecordType<R> recordType, Class<? extends E> type) {
        if (type == A.class)
            return record -> (E) new DefaultRecordMapper<>(recordType, A.Builder.class, configuration).map(record).build();
        if (type == B.class)
            return record -> (E) new DefaultRecordMapper<>(recordType, B.Builder.class, configuration).map(record).build();
        else
            return new DefaultRecordMapper<>(recordType, type, configuration);
    }
});

@eduramiba
Copy link
Author

@knutwannheden Awesome, thank you! I will probably do that until #6598 is ready :)

@lukaseder lukaseder added this to To do in 3.13 Other improvements via automation Jan 16, 2020
@lukaseder lukaseder self-assigned this Jan 16, 2020
3.13 Other improvements automation moved this from To do to Done Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

3 participants