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

Support one-to-many mappings in sqlobject #996

Closed
rbkrabbe opened this issue Jan 5, 2018 · 20 comments
Closed

Support one-to-many mappings in sqlobject #996

rbkrabbe opened this issue Jan 5, 2018 · 20 comments

Comments

@rbkrabbe
Copy link

rbkrabbe commented Jan 5, 2018

As far as I can tell, one-to-many relations can be mapped using either

I would like to be able to return custom beans, containing one-to-many relations, from sqlobject.

Using the example at http://jdbi.org/#_joins i would like to be able to do something similar to the following:

public interface ContactDao {

    @SqlQuery("select contacts.id c_id, name c_name, "
        + "phones.id p_id, type p_type, phones.phone p_phone "
        + "from contacts left join phones on contacts.id = phones.contact_id "
        + "order by c_name, p_type ")
    List<Contact> getAll();

    @SqlQuery("select contacts.id c_id, name c_name, "
        + "phones.id p_id, type p_type, phones.phone p_phone "
        + "from contacts left join phones on contacts.id = phones.contact_id "
        + "order by c_name, p_type where contacts.id = :id")
    Optional<Contact> getOne(Long id);

    @SqlUpdate("insert into contacts (id, name) values (:id, :name)")
    void addContact(Long id, String name);
}

//More methds here.

I can see 2 ways this might be implemented:

  • Add a ResultSetMapper type, that can manually accumulate the results by mapping the entire ResultSet
  • Add a RowReducer type (analogous to the BiFunction used by reduceRows), that can be registered in sqlobject.

If someone can help me get started, I would be happy to contribute the code myself, but I would need some guidance as to which implementation is preferred.

@qualidafial
Copy link
Member

First off, you can always extend SqlObject in your SQL Object interfaces, and use default methods to "drop down" to core for reducing. So while it is not as convenient as annotated @SqlQuery, it is available to you in SQL Objects.

Notwithstanding, I agree this is a shortcoming, and one I'd love to see solved. Be warned any proposed solution would have to be pretty generic and adaptable to different usage patterns for us to adopt it into the project. This is a whole can of worms we have to think about:

  • Instantiation patterns
    • Default constructors
    • All args constructors
    • Static factory methods
    • Builder classes
  • Accessors and mutators patterns
    • Regular getters and setters e.g. getFoo(foo), setFoo(foo).
    • withFoo(foo) methods, often return a new instance of the self type
    • addFoo(foo) mutator methods that regulate access to the underlying List<Foo>
    • Self-titled getters and setters, e.g. void foo(Foo foo) {this.foo = foo;}, Foo foo() {return foo;}

Ideally we'd want to structure this in a way that promotes reuse of registered mappers without having to reinvent the wheel for every possible corner case.

I think a productive place to start is to ask the question: what annotations would you like to exist, how would they look on a SQL Object interface, and how would those annotations be applied in the mapping logic behind the scenes?

@qualidafial
Copy link
Member

We also need a way to tell Jdbi which columns uniquely identify the master record, and which identify each child record within each row, to identify coalesce duplicates.

And how should we deal with deeply nested collections? e.g.

class A {
  List<B> b;
}

class B {
  List<C> c;
}

class C {
   ...
}

@rbkrabbe
Copy link
Author

rbkrabbe commented Jan 7, 2018

While a general approach might be more performant, I think that for most cases it would be sufficient to be able to register a reducer, that calls reduceRows and returns the result.

The annotations would look something like this:


public interface ContactDao {

    @SqlQuery("select contacts.id c_id, name c_name, "
        + "phones.id p_id, type p_type, phones.phone p_phone "
        + "from contacts left join phones on contacts.id = phones.contact_id "
        + "order by c_name, p_type ")
    @RegisterRowReducer(ContactReducer.class)
    List<Contact> getAll();

    @SqlQuery("select contacts.id c_id, name c_name, "
        + "phones.id p_id, type p_type, phones.phone p_phone "
        + "from contacts left join phones on contacts.id = phones.contact_id "
        + "order by c_name, p_type where contacts.id = :id")
    @RegisterRowReducer(OptionalContactReducer.class)
    Optional<Contact> getOne(Long id);

    @SqlUpdate("insert into contacts (id, name) values (:id, :name)")
    void addContact(Long id, String name);
}

With the row reducer looking something like this:

public interface RowReducer<TSeed, TResult> {
    TSeed getSeed();
    BiFunction<TSeed, RowView, TSeed> getAccumulator();
    TResult getResult(TSeed accumulated);
}

getSeed and getAccumulator are used as inputs to reduceRows, and when all rows have been processed, getResult is called, to extract the target type from the accumulated values.

Would that make sense?

@qualidafial
Copy link
Member

What if instead of a generic TResult type, we returned a stream of result R elements?

interface RowProcessor<A, R> {
  A getSeed();
  A apply(A accumulator, RowView rowView);
  Stream<R> getResult(A accumulator);
}

Example implementation:

class ContactPhoneRowReducer implements RowReducer<Map<Long,Contact>, Contact> {

  Map<Long,Contact> getSeed() {
    return new LinkedHashMap<Long, Contact>();
  }

  Map<Long,Contact> apply(Map<Long,Contact> accumulator, RowView rowView) {
    accumulator.computeIfAbsent(rowView.getColumn("c_id", long.class),
                                id -> rowView.getRow(Contact.class))
        .getPhones()
        .add(rowView.getRow(Phone.class));
    return accumulator;
  }

  Stream<Contact> getResult(Map<Long, Contact> acc) {
    return acc.values().stream();
  }
}

We could provide an abstract implementation based on a LinkedHashMap accumulator that takes care of all the boilerplate, so you only have to write the accumulator function:

class ContactPhoneRowReducer extends MapAccumulatorRowReducer<Long, Contact> {
  void accept(Map<Long,Contact> accumulator, RowView rowView) {
    accumulator.computeIfAbsent(rowView.getColumn("c_id"),
                                id -> rowView.getRow(Contact.class))
        .getPhones()
        .add(rowView.getRow(Phone.class));
  }
}

In the SQL Object interface, we'd need an annotation that tells Jdbi that this method's result is reduced from the RowView, instead of mapped row by row:

public interface ContactDao {
    @SqlQuery("select contacts.id c_id, name c_name, "
        + "phones.id p_id, type p_type, phones.phone p_phone "
        + "from contacts left join phones on contacts.id = phones.contact_id "
        + "order by c_name, p_type ")
    @ReduceRows(ContactPhoneRowReducer.class)
    List<Contact> getAll();

    @SqlQuery("select contacts.id c_id, name c_name, "
        + "phones.id p_id, type p_type, phones.phone p_phone "
        + "from contacts left join phones on contacts.id = phones.contact_id "
        + "order by c_name, p_type where contacts.id = :id")
    @ReduceRows(ContactPhoneRowReducer.class)
    Optional<Contact> getOne(Long id);
}

This annotation would apply to methods with the following annotations:

  • @SqlQuery
  • @SqlUpdate + @GetGeneratedKeys
  • @SqlBatch + @GetGeneratedKeys

Since the reducer produces a Stream<Contact>, the return type can be any container type registered with SQL Object through JdbiCollectors.

I could see RowReducer being part of core, with a new method in ResultBearing:

default <T> Stream<T> reduceRows(RowReducer<?, T> reducer)

@jdbi/contributors What do you think?

@qualidafial
Copy link
Member

I'm doing a spike on this, so far so good 🤞

@stevenschlansker
Copy link
Member

stevenschlansker commented Jan 8, 2018

Can the RowReducer interface be implemented as a Collector<RowView, ?, T>, then you can just reuse the existing Collector functionality? Or are there things that can be uniquely reduced but not collected? (where instead of a 'result container', you abuse it as the 'accumulator')

@qualidafial
Copy link
Member

I looked into that--there are strong similarities, but Collector tends to be implemented with lambdas instead of as a defined class, using Collector.of(supplier, accumulator, combiner, finisher).

@stevenschlansker
Copy link
Member

stevenschlansker commented Jan 8, 2018

I don't think "it's usually done with lambdas" is a sufficient reason to re-invent an interface, assuming it actually would end up working out. RowReducer as sketched above sure looks almost equivalent to collecting.

@qualidafial
Copy link
Member

This may be the tail wagging the dog, but one reason I favor our own interface over Collector is so the compiler can statically validate the RowReducer. We can't quite get there with Collector because of nested generic variables:

@interface UseRowReducer {
    Class<? extends Collector<RowView, ?, Stream<?>>> value();
}

interface Good extends Collector<RowView, Map<Long, Stream>, Stream<String>> {}
interface BadInput extends Collector<String, Map<Long, Stream>, Stream<String>> {}
interface BadResult extends Collector<RowView, Map<Long, Stream>, List<String>> {}
interface Pointless extends Collector<RowView, String, Stream<?>> {}

interface TestAnnotations {
    @UseRowReducer(Good.class)
    void good();

    @UseRowReducer(BadInput.class)
    void badInput();

    @UseRowReducer(BadResult.class)
    void badResult();

    @UseRowReducer(Pointless.class)
    void pointless();
}

Out of the above method annotations, only @UseRowReduce(Pointless.class) compiles. Good.class is one we want to work but doesn't due to limitations of Java's generics.

@rbkrabbe
Copy link
Author

rbkrabbe commented Jan 9, 2018

JdbiCollectors are pretty much undocumented (http://jdbi.org/#JdbiCollectors is referenced in the documentation, but the anchor does not exist), and it is not cear how they integrate with SqlObject, so they are kind of hard to use right now.

OTOH it is clear that there is a lot of overlap between Collectors and RowReducers, and for my uses a Collector<RowView, ?, T> would be fine. This would also allow collecting into a single entity without having to unwrap it from the stream.

I think that what I would like the most is a new method on ResultBearing

Stream<RowView> streamRows()

along with some way of using it from SqlObject, as that would allow me to combine the mapping capabilities of the RowView with the full flexibility of the filtering, mapping and collecting built into the Streams api.

@qualidafial
Copy link
Member

I'm concerned that if we provide a Stream<RowView> then users will think they can just store the RowView objects from the stream and have them be stable data containers--when in fact there is only one RowView instance per ResultSet, but it becomes a view over different rows as you advance the ResultSet.

@stevenschlansker
Copy link
Member

stevenschlansker commented Jan 9, 2018

In fact, any stateful Stream operation (such as sorted or distinct) will do this. So I think unfortunately that idea is a non-starter. That's why it was attractive to me to hook into Collector -- you get the advantage of reusing one of the most advanced parts of the Stream library, but without this huge gotcha.

@stevenschlansker
Copy link
Member

stevenschlansker commented Jan 9, 2018

And yeah, we really need to document Collectors. I'll try to do that this week. But it should mostly be a, "read the Collectors documentation, and here's a couple of examples" -- Jdbi does not really introduce new concepts on top of it, only exposes it through the API.

@qualidafial
Copy link
Member

What if we used Collector in core, and RowReducer was a SQL Object-only type (to make annotation-driven reducers easy to express), and provided a method to convert them to collectors?

interface ResultBearing {
    <A, R> R collectRows(Collector<RowView, A, R> collector);
}
public interface RowReducer<A, R> {
    A supplyAccumulator();
    void accumulateRow(A accumulator, RowView rowView);
    Stream<R> toStream(A accumulator);
    // but with better method names :)

    default Collector<RowView, A, Stream<R>> collector() {
        return Collector.of(
                this::supplyAccumulator,
                this::accumulateRow,
                (a, b) -> {
                    throw new UnsupportedOperationException("RowReducer does not support parallel streams");
                },
                this::toStream);
    }
}

@stevenschlansker
Copy link
Member

stevenschlansker commented Jan 12, 2018 via email

@qualidafial
Copy link
Member

I'm definitely open for suggestions on the RowReducer method names though.. :)

@stevenschlansker
Copy link
Member

stevenschlansker commented Jan 12, 2018

How about ResultCollector or ResultReducer? and then container() (or accumulator()) collect() (or accumulate()) and finish() (or just stream())

(the type really operates over all results, not a single row)

@rbkrabbe
Copy link
Author

How about RowAccumulator along with @AccumulateRows?

Wrt. the method names, I think it would be wise to draw on the naming fro the Streams API in order to foster recognition. That would mean names like container/seed, accumulator and finisher (but I agree that stream would be a good alternative, seeing as the type is not optional in this case.

@qualidafial
Copy link
Member

qualidafial commented Jan 13, 2018

So far we have one method dealing with RowView named reduceRows(). For consistency, I favor names like collectRows(), accumulateRows(), reduceRows(), etc for anything dealing with a RowView. I lean toward collectRows() for the method, since it's using a Collector.

Ditto for RowReducer--and I do think that one is a reducer, since you're trying to reduce a master-detail join into one or more master objects.

I keep going back and forth though on whether RowReducer should be in SQL Object, or be in core and have a method in ResultBearing:

<A, R> R collectRows(Collector<RowView, A, R> collector);

<A, R> Stream<R> reduceRows(RowReducer<A, R> reducer);

I lean toward core, partly so the same reduce mechanism may be used in either core of SQL Object (consistency, reuse), and partly because LinkedHashMapRowReducer makes the 90% case of master-detail reductions dirt simple in core (ease of use):

List<Contact> result = dbRule.getSharedHandle()
    .createQuery("SELECT ... FROM contacts LEFT JOIN phones on ...")
    .reduceRows(LinkedHashMapRowReducer.<Integer, Contact>of((map, rv) ->
        Contact contact = map.computeIfAbsent(
                rv.getColumn("contact_id", Integer.class),
                id -> new SomethingWithLocations(rv.getRow(Contact.class)));
        if (rv.getColumn("phone_id", Integer.class) != null) {
          contact.getPhones().add(rv.getRow(Phone.class));
        }
    }) // returns Stream<Contact>
    .collect(toList());

@qualidafial
Copy link
Member

Should we support @UseRowReducer with @SqlBatch and @SqlUpdate? I have a working implementation but I can't fathom any use cases.

qualidafial added a commit that referenced this issue Jan 18, 2018
* RowReducer<A, R> interface.
* LinkedHashMapRowReducer abstract implementation.
* ResultBearing.collectRows(Collector<RowView,A,Stream<R>>)
* ResultBearing.reduceRows(RowReducer<A,R>)
* @UseRowReducer annotation.
* Refactor tests to demonstrate usage

TODO:

* Decide whether to support @UseRowReducer with @SqlBatch, @SqlUpdate
* Document reducers in developer guide.
@qualidafial qualidafial mentioned this issue Jan 18, 2018
3 tasks
qualidafial added a commit that referenced this issue Jan 18, 2018
* RowReducer<A, R> interface.
* LinkedHashMapRowReducer abstract implementation.
* ResultBearing.collectRows(Collector<RowView,A,Stream<R>>)
* ResultBearing.reduceRows(RowReducer<A,R>)
* @UseRowReducer annotation.
* Refactor tests to demonstrate usage

TODO:

* Decide whether to support @UseRowReducer with @SqlBatch, @SqlUpdate
* Document reducers in developer guide.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants