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

Wrong result if copying set in the constructor #487

Closed
pawelgosztyla opened this Issue Feb 14, 2018 · 3 comments

Comments

Projects
None yet
2 participants
@pawelgosztyla
Copy link

pawelgosztyla commented Feb 14, 2018

Hi,
I'm not sure if it is related to #476, if yes then sorry for duplicate.
I am using JdbcMapper.stream to map one-to-many relation to my objects:

class A {
  private Set<B> bs;
}
class B {
}

I am using builder pattern to instantiate objects of A and I'm copying the set in the constructor to guarantee that A is immutable:
private A(Set<B> bs) { this.bs = Collections.unmodifiableSet(new HashSet<>(bs)); }

In the result there is an A with only one object of B in the set whilst there should be more of them.

After some of debugging I found that the sequence of actions is as follows:

  1. new set of Bs is created and one object is inserted
  2. instance of A is instantiated using builder, the set with one element inside is used
  3. the set is copied in the A's constructor
  4. more objects of B are added to the set
@arnaudroger

This comment has been minimized.

Copy link
Owner

arnaudroger commented Feb 14, 2018

Yes it’s link to 476 have not come to try to sort it out, but I’ll have a look.

@arnaudroger

This comment has been minimized.

Copy link
Owner

arnaudroger commented Feb 15, 2018

so I had a think about it, it won't be really trivial to do so might take some time....

for jdbc the problem is that we only know we are on the next root object once we are there and by them the ResultSet does not point to the previous object.
so we need an intermediary structure to store the information are we need to call previous on the ResultSet but there is no guarantee the ResultSet are any other tuple source will support that, so not really an option.

My thinking now is that the JoinMapper will map to an intermediary builder that will be use as a source for the instantiator - or mapper.
My hope is that we can than converge the csv mapping code with the jdbc allowing for a pull/push convergence.

@arnaudroger arnaudroger added this to the 4.0.0 milestone Jun 13, 2018

@arnaudroger arnaudroger closed this Jul 9, 2018

@arnaudroger

This comment has been minimized.

Copy link
Owner

arnaudroger commented Aug 14, 2018

so pushed 4.0.0 last night it should fixed that, but you will need to add @ModifyInjectedParams annotation to the class - I meant to have flag in the mapper but it's not accessible will be fix in 4.0.1 where you will need to call assumeInjectionModifiesValues() on the factory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.