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

Quarkus sql client PreparedQuery.executeBatch number of rows affected always equal to 1 #16338

Closed
Tracked by #16955
lucafuji opened this issue Apr 7, 2021 · 13 comments · Fixed by #17076
Closed
Tracked by #16955
Labels
kind/bug Something isn't working
Milestone

Comments

@lucafuji
Copy link

lucafuji commented Apr 7, 2021

Describe the bug

We are using io.quarkus:quarkus-reactive-pg-client library and quarkus 1.11.1.Final.
We are using the preparedQuery.executeBatch as below, but the rowCount always returns 1 no matter how many rows it updated

import io.vertx.mutiny.sqlclient.RowIterator;
import io.vertx.mutiny.sqlclient.RowSet;
import io.vertx.mutiny.sqlclient.SqlClient;
import io.vertx.mutiny.sqlclient.SqlResult;
import io.vertx.mutiny.sqlclient.Tuple;

  default Uni<Integer> batchUpdate(SqlClient client, String sql, List<List<Object>> paramList) {
    return client.preparedQuery(sql).
        executeBatch(prepareBatchParams(paramList)).
        map(SqlResult::rowCount);
  }

Expected behavior

The row count should return actual number of rows updated

Actual behavior

Always returns 1

@lucafuji lucafuji added the kind/bug Something isn't working label Apr 7, 2021
@geoand
Copy link
Contributor

geoand commented Apr 8, 2021

cc @tsegismont

@tsegismont
Copy link
Contributor

Thanks for the heads-up @geoand . I'll look into it

@tsegismont
Copy link
Contributor

@lucafuji this might not be a bug.

The rowCount method returns the affected rows for each RowSet. If the query executed affects one row per batch, then calling rowCount on each RowSet will return 1.

Can you try to iterate using RowSet.next until it returns null, and verify RowSet.rowCount returns 1 every time?

@lucafuji
Copy link
Author

lucafuji commented Apr 9, 2021

@tsegismont It seems the returned row set is empty, I made the following changes:

default Uni<Integer> batchUpdate(SqlClient client, String sql, List<List<Object>> paramList) {
    return client.preparedQuery(sql).
        executeBatch(prepareBatchParams(paramList)).
        map(rows -> rows.iterator().next().getInteger(0)));
  }

and immediately got a NoSuchElementException

"java.util.NoSuchElementException: null\n\tat java.util.ArrayList$Itr.next(ArrayList.java:862)\n\tat io.vertx.sqlclient.impl.RowSetImpl$1.next(RowSetImpl.java:74)\n\tat io.vertx.mutiny.sqlclient.RowIterator.next(RowIterator.java:71)\n

@tsegismont
Copy link
Contributor

@lucafuji it is expected that the rowset is null if the query does not return results (e.g. INSERT or UPDATE without RETURNING clause). I guess this is why you get a NoSuchElementException by calling next() on the rowset iterator.

My suggestion was to invoke next() on the result directly:

client.preparedQuery(sql)
        .executeBatch(prepareBatchParams(paramList))
        .map(res -> {
                int total = 0;
                do {
                  total += res.rowCount();
                } while ((res = res.next()) != null);
                return total;
        });

@geoand
Copy link
Contributor

geoand commented Apr 12, 2021

@tsegismont is your suggestion something we should document somewhere?

@tsegismont
Copy link
Contributor

@geoand we could do that. First let's see if this is a misuse of the API or actually a bug.

@lucafuji
Copy link
Author

seems working now @tsegismont thanks a lot

@zaneschepke
Copy link

zaneschepke commented Apr 13, 2021

For anyone wanting to have all of the actual inserted results from their RETURNING * query instead of just rowCount, this seems to work.
client.preparedQuery(sql).executeBatch(batch).map(res -> { Set<Row> rows = new HashSet<>(); do { res.forEach(rows::add); } while ((res = res.next()) != null); return rows; }) .onItem().transformToMulti(set -> Multi.createFrom().iterable(set)) .onItem().transform(MyObject::from);

@tsegismont
Copy link
Contributor

@zaneschepke this would work too:

        client.preparedQuery(sql).executeBatch(batch)
                .onItem().<RowSet<Row>>transformToMulti(res ->
                    // Generate a Multi of RowSet items
                    Multi.createFrom().generator(() -> res, (rs, emitter) -> {
                        RowSet<Row> next = null;
                        if (rs != null) {
                            emitter.emit(rs);
                            next = rs.next();
                        }
                        if (next == null) {
                            emitter.complete();
                        }
                        return next;
                    })
                )
                // Transform each RowSet into Multi and Concatenate 
                .onItem().transformToMultiAndConcatenate(Multi.createFrom()::iterable)
                // Transform each Row into MyObject
                .onItem().transform(MyObject::from);

@cescoffier
Copy link
Member

@tsegismont should this be documented?

@tsegismont
Copy link
Contributor

@cescoffier where would be document this?

@cescoffier cescoffier mentioned this issue May 3, 2021
7 tasks
@cescoffier
Copy link
Member

tsegismont added a commit to tsegismont/quarkus that referenced this issue May 7, 2021
@quarkus-bot quarkus-bot bot added this to the 2.0 - main milestone May 7, 2021
@gsmet gsmet modified the milestones: 2.0 - main, 1.13.4.Final May 10, 2021
gsmet pushed a commit to gsmet/quarkus that referenced this issue May 10, 2021
gsmet pushed a commit to gsmet/quarkus that referenced this issue May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants