ADBA feedback

[Lukas Eder]

Hello,

I finally got around to playing with ADBA and AoJ and I'd like to provide
some feedback. I will play around with it a bit more in the near future,
perhaps waiting for new iterations. At this point, I'd like to thank you
very much (again) for your open development approach. I find this way of
looking into your work and being able to provide feedback (and your
openness to said feedback) very constructive!

AOJ:
=========================

Setup:
-------------------------

I found it a bit difficult to set up - it took me around 1/2h to fix all
the dependency issues / project setup, both with Eclipse and IntelliJ.
Haven't tried NetBeans. I got it working but unfortunately, when I'll try a
future version of AOJ / ADBA, I'm afraid I'll have to go through that setup
hassle again, which isn't too motivating :-)

Is there any chance to provide an out-of-the-box working AOJ project that
is built with e.g. Maven and that doesn't need any tweaking but should work
out of the box with Maven and popular IDEs? I think this would help people
provide even more feedback

Bugs:
-------------------------

I have also provided PRs on GitHub, but haven't had time to read through
the OCA yet, so I'll just document the bugs here as well, in case you want
to reject the PRs for legal reasons.

1) https://github.com/oracle/oracle-db-examples/pull/37

The RowOperation.getIdentifiers() method is implemented incorrectly with an
off-by-one error. JDBC is 1-based, the implementation here is 0-based.

2) https://github.com/oracle/oracle-db-examples/pull/36

There are quite a few compilation errors (see also setup problems above)
due to the fact that the linked ADBA snapshot version seems to be out of
date or too far advanced for the AoJ code dump. The PR illustrates just one
example, but there are more.

3) (no github issue)

Resource handling within AOJ could be better, e.g.
RowOperation.completeQuery() doesn't correctly close the resources. I'm not
sure if this matters, but the code doesn't expose best JDBC practices, at
times :-)

Questions about code usage:
-------------------------

1) What are all these trailing getCompletionStage() calls for? Are they
really needed? Calling submit() as the last operation seems sufficient, no?
Probably from earlier versions where the CompletionStage was assigned to
some variable?


ADBA:
=========================

Bikeshedding on naming:
-------------------------

(The least important thing first)

These things have bothered me less before I started playing with AoJ but
now they become a bit more apparent:

1) collect() feels weird. Stream.collect() is a synchronous, terminal op.
RowOperation.collect() is a lazy operation. Perhaps collecting() is better,
here? I don't know, just wanted to provide feedback that the term collect
itself has confused me at first.

2) Submission.getCompletionStage() breaks the fluent API for me.
Submission.cancel() makes much more sense, for instance. All through the
call chain, we have nice verbs like set(), collect(), submit(), cancel()
and then this weird getCompletionStage(). But I don't have a better name
proposal.

3) CountOperation seems wrong. Why not UpdateOperation (like JDBC's
executeUpdate()), or at even better: RowCountOperation? Because
  o The term "count" implies to me that a SELECT COUNT(*) query was run
  o not all "Count" operations actually return a meaningful rowcount, e.g.
DDL statements

Design concerns:
-------------------------

1) Result.Row should have better methods akin to what JDBC's
ResultSetMetaData (RSMD) has. For instance:
  o It's currently not possible to discover individual column types
  o The getIdentifiers() array corresponds to the RSMD.getColumnName()
values, but what about qualification of columns? What about
RSMD.getColumnLabel()?
  o Nullability flags may be cool too. Not sure about the other info in RSMD

2) A Result.Row.get(int, ...) overload to Result.Row.get(String, ...)
should be added in my opinion. E.g. what if I run this query? SELECT 'x' AS
"5", 'y' AS "1" FROM dual, and now I'm accessing Row.get("1"). Which row
should be returned? The row with index 1, or the row with name "1"? I'm not
happy with the existing "lazy" API design approach that prevents this
overload. The overload would remove any ambiguity.

3) I find the onError() vs submit().getCompletionStage().exceptionally()
duality a bit confusing. Both seem to work, so is onError() really needed?
If so, what's the difference?

4) I think the Result type should have a reference to its "parent" /
producing type, e.g. Connection. The example where the countOperation()
rolls back a transaction by referencing it from the lambda's outer scope
shows a flaw (IMO an important one) of the current design. The transaction
(and connection) should be available from the lambda's Result.Count
argument as a contextual parameter, for a much more functional design, not
in a globalish-variable kind of style. This is especially important in an
asynchronous API, where the statement submission may happen entirely
elsewhere than the rollback decision.

5) I have many doubts about the Transaction design:
  o What is the meaning of the word "Only"? Why not just call it
Transaction.rollback()
  o What is the utility of the getter "isRollbackOnly()"?
  o The API seems difficult to use correctly (probably even harder to
implement correctly). Why does Connection.catchErrors() have to be called?
  o What happens if Connection.commitMaybeRollback(Transaction) is
forgotten? Will the transaction be committed / rolled back implicitly when
the Connection is closed?
  o What about savepoints, which are essential for the implementation of
nested transactions? Are they omitted deliberately or is their absence an
oversight?
  o Is there any real utility for this Transaction type (in the absence of
a Savepoint), given that the commit and commitMaybeRollback methods are on
the Connection only
  o What happens when a Transaction.setRollbackOnly() call is made, but
then Connection.commit() "overrides" it?
  o What happens when Connection.transaction() is called twice? Will we get
two times the same transaction reference, or will two separate,
"autonomous" transactions be spawned?

6) Why is Row.rowNumber() zero based? That's very confusing. E.g. Row.get()
is still 1-based, and all SQL databases that support either ROWNUM or
ROW_NUMBER() OVER () functions will produce 1-based row numbers

7) I have to try my luck again on those type witnesses that are required on
each and every statement call. They are very annoying (and hard to get
right or be properly understood by beginner developers who struggle with
Java generics already) and I don't think they are necessary. I think the
collect() method should define the type of the result, and in the absence
of a collect() method, the result type should be a wildcard ?, or
java.lang.Void. Just like a Collector<T, A, R> passed to
Stream<T>.collect() will return R, and not T, I think a call to
RowOperation<?>.collect() should eventually produce the R type.

If I remember correctly, the current design was there to simplify the API
as it is now. But I really think the price to pay for this is very very
high. There should be no type witness needed.

8) While I still like the idea of using a Collector to retrieve the results
(and I don't want to engage in the discussion about the possibility of
using Flow style streams instead), I think that there should be either:

  o Some convenience overloads like this

    public default RowOperation<T> consume(Consumer<? super Result.Row> c) {
        return collect(Collector.of(() -> null, (a, r) -> c.accept(r), (x,
y) -> null));
    }
  o Some public "common Collector" utility like this:

    Collector<Result.Row, Void, Void> consume(Consumer<Result.Row>
consumer) {
        return Collector.of(() -> null, (a, r) -> consumer.accept(r), (x,
y) -> null);
    }

The example code shown in AoJ is a bit scary. If for the most simple
queries, we need to write those Collectors with all the types and dummy
suppliers and combiners, that'll scare off people. Besides, there's no need
to reinvent the wheel as a user, so these libraries should be provided.

Another convenience tool could be a toList() collector, or default method
on RowOperation:

    public default <S> RowOperation<List<S>> toList(Function<? super
Result.Row, ? extends S> f) {
        return collect(Collector.of(
            () -> new ArrayList<S>(),
            (List<S> a, Result.Row r) -> {
                a.add(f.apply(r));
            },
            (l, r) -> { l.addAll(r); return l; }
        ));
    }

There might be many more.

Thanks,
Lukas