jdaugherty commented on PR #16619:
URL: https://github.com/apache/grails-core/pull/16619#issuecomment-6258626590
Thanks — this was a thorough review and every point was actionable. `9.0.x`
is merged (all 14 conflicts resolved) and all six items plus the smaller ones
are addressed. Details below, grouped as you numbered them.
## 1. The rebase
Merged `9.0.x`. The four conflicts that needed more than keeping both sides:
- **`upgrading90x.adoc`** is now one page: the skill TIP, then `1. Groovy
6`, `2. GPars Integration Deprecated in Favour of Groovy Concurrency`, `3.
Groovy Code Is Compiled with invokedynamic`, `4. grails.converters.JSON ...`,
`5. Domain Classes Can Render Through a Jackson Serializer`, `6. RxGORM Runs on
RxJava 3`, `7. RxJava 1 and RxJava 2 ... Deprecated`. I kept your relative
ordering (Groovy 6 → GPars → RxGORM → RxJava deprecation) and left the three
sections `9.0.x` already had in their existing order, so the renumbering is a
shift of one rather than a reshuffle. The GPars section no longer opens by
re-announcing the Groovy 6 baseline, since section 1 now does that.
- **The `grails-9-upgrade` skill** gained the GPars material — a section
with the one-for-one dependency replacement, the `CompletableFuture` failure
contract the Groovy factory follows, the actor delivery semantics, a
`GParsPool`/`GParsConfig` → `groovy.concurrent` table, checklist items (no
`grails-async-gpars`/`grails-events-gpars`, no `groovyx.gpars` import, no
`GParsPool.withPool` or `GParsConfig`) and a pitfall about keeping both
modules. Your argument applies equally to the other two sections `9.0.x` added,
so I also gave it `Groovy Code Compiles with invokedynamic` (the `grails.indy`
property, `groovy-callsite` per source set, the GraalVM constraint, the
`testcontainers-spock` case) and `JSON Rendering Can Use Spring Boot's
JsonMapper` (the `legacy` setting, the values that change, marshaller
precedence, `JSONWriter` subclassing, `DomainClassSerializer`). The
`description` and the workflow steps cover all of it now.
- **`asyncServerSentEvents.adoc`** stays deleted, and `rxjava.adoc` picks up
the pointer. "RxJava Results from Controllers" now leads with the fact that an
RxJava 3 `Flowable` *is* a `Publisher`, shows an action returning
`observable.toFlowable(BackpressureStrategy.BUFFER)` and links
`<<asyncStreaming,Streaming Responses>>`, then says plainly that `Observable`
and `Single` are not `Publisher`s and must be converted. The NOTE no longer
claims the plugin was needed for that, and it names both
`org.grails.plugins:rxjava` and `org.grails.plugins:rxjava2`.
I also corrected `asyncStreaming.adoc`. You're right that
`PublisherActionResultTransformer` gates on `actionResult instanceof Publisher`
(line 82) before `isSingleValue` ever consults the `ReactiveAdapterRegistry`,
so the RxJava `Single` example was wrong. That line now names only `Mono` and a
`CompletableFuture` adapted to one, and a following paragraph says a
non-`Publisher` result is left alone, and that a `Flowable` is multi-valued
whatever it emits — so a one-item `Flowable` streams as a single event rather
than resolving.
- **The tables and `asyncConfigure.adoc`** now match the GPars style. The
RxJava rows in `asyncPromises.adoc` read "(deprecated, see below)" like the
GPars row, and the two `WARNING` blocks are gone, replaced by a prose paragraph
next to the GPars paragraph in each of `asyncPromises.adoc` and `events.adoc`.
The `asyncConfigure.adoc` TIP is merged: RxJava 3 first, then the Groovy actor
bus through `Awaitable.setExecutor`, then the deprecated GPars bus, then the
deprecated RxJava 1 and 2 buses.
The skill-registration conflicts kept both `grails-ai-developer` and
`grails-9-upgrade`.
## 2. `insert()` mapping to `null`
Fixed — `insert()` uses the same `flatMap` with `Observable.empty()` that
`persist()` does, so both complete empty. Three tests in
`AbstractRxDatastoreClientSpec`: the empty path of each, and the non-empty path
of `insert()` for symmetry with the existing `persist()` ones. The two
empty-path tests fail on the old code, so they pin the fix rather than just
describing it.
On the wording: you're right that the old example described a case that
actually emits. `upgrading90x.adoc` now reads "`save()` and `insert()` complete
empty, instead of emitting `null`, when the datastore reports no identifier for
the instance", which is the condition the code actually tests.
## 3. The IO scheduler
I kept `observeOn` and fixed the docs, rather than switching to
`subscribeOn`. The reason is the one your probe implies but doesn't force:
`RxServiceSupport` does no session or transaction propagation. Asynchronous
GORM wraps its work in a new session precisely because it moves the call to
another thread; `subscribeOn` here would move a blocking GORM call to an IO
thread with no bound Hibernate session and nothing the tenant resolver reads
from the request, so a working-if-pointless feature would become a broken one,
silently, inside a port. That's worth its own change with session propagation,
a thread-asserting test and an upgrade-guide entry — I'd rather not smuggle it
in here. Happy to file it.
So the docs now say what the code does, in all the places you listed and a
couple more:
- `rxjava.adoc:48` — the query runs on the subscribing thread and the result
is delivered on the IO scheduler.
- `rxjava.adoc:71` — `@RxSchedule` "selects the scheduler a method delivers
its result on", with an explicit sentence that neither it nor the default moves
the query off the subscribing thread, and a pointer to subscribing from a task.
- `rxServices.adoc:54` in both Hibernate guides, and the "run the operation
on a different scheduler" line below it.
- The javadocs on `RxServiceSupport` (class and all four methods) and
`RxScheduleIOTransformation`, plus the `@RxSchedule` annotation javadoc, which
made the same claim.
- `asyncRxGorm.adoc`, which compared the data services to Asynchronous GORM
"isolating blocking operations on a separate thread" — now says they do *not*
do that, unlike Asynchronous GORM.
And the thread is now tested, both ways, with `Schedulers.from(executor)`
and a named thread factory: `create` and `createSingle` each assert the
callable ran on the subscriber's thread and the emission arrived on
`rx-delivery` / `rx-single-delivery`. If the behaviour is ever changed to
`subscribeOn`, those two tests are what fails.
## 4. The `rx-mongodb` feature
Removed in this PR — `feature.yml`, its skeleton, and the feature from the
six profile pages that listed it. An application generated with it could never
resolve `org.grails.plugins:rx-mongodb`, so it seemed worse to ship 9.0 with it
than to take it out here, especially as this PR was editing it.
`upgrading90x.adoc` and the skill now say the feature is removed and why,
instead of presenting it as usable.
## 5. Doc pages contradicting the description
- `introduction.adoc:28` rewritten: no `RxEntity` datastore implementation
ships, RxGORM for MongoDB was last released for GORM 6.1 on RxJava 1, Hibernate
cannot provide one, use the RxJava data services.
- `agentSkills.adoc` — the heading is "Applications on the Previous Major
Version" and the line below the example says "Once the application is on Grails
9".
- `rxjava.adoc:83-90` — the example is now `respond
bookService.findAllByTitle(title).firstElement().blockingGet()`, which gives
`null` for a missing title and so a 404; I added `findAllByTitle` to the
service interface above it so the example resolves, and said in the prose why
`firstElement()` rather than the `Single`.
## 6. Tests for the new behaviour
All four gaps covered; `:grails-datamapping-rx:test` is 514 (was 503) and
`RxServiceImplSpec` 9 (was 7), all passing.
- `createSingle` with a `null` result → `NoSuchElementException`, both as a
unit test in `RxServiceSupportSpec` and end to end in `RxServiceImplSpec` via
`findOne('Missing')`, plus the `Observable` counterpart asserting an empty
completion and that `firstElement().blockingGet()` is `null`.
- The empty paths of `persist()` and `insert()` (see 2).
- `ObservableSource` composition, since What's New and the guide advertise
it: `Observable.wrap(criteria)` and `concatWith(criteria)` in
`DetachedCriteriaSpec`, the same pair against a real proxy in
`RxJavassistProxyFactorySpec`, and both operators against a lazy
`RxPersistentSet` — that last one without any blocking initialization, which is
the point.
- The thread the callable runs on (see 3).
## Smaller items
- **Warning logged per instance** — both promise factories and both event
buses now use a `private static final AtomicBoolean DEPRECATION_LOGGED` with
`compareAndSet`, matching `GparsPromiseFactory`. Each spec resets the latch in
`setup()` so the existing `appender.list.size() == 1` assertions still mean
something, and each gained a feature that constructs three instances and
asserts exactly one warning — which is the behaviour you asked for, so it's
pinned rather than implied. All four module suites pass.
- **`whereQueries.adoc:32-43`** — the example is `.findAll().subscribe { ...
}`, and a NOTE explains that a closure cannot be subscribed to an
`ObservableSource` because `Observer` has four methods. The paragraph above now
describes the criteria as an `ObservableSource` that composes, which is the
accurate claim.
- **`IdQueryObservableProxyMethodHandler.java:77`** — branch dropped, with a
comment noting that RxJava 3 never emits `null` so the function isn't called at
all for an empty query.
- **`SKILL.md:111`** — reworded: declare `Observable` and use
`firstElement()`, which gives a `Maybe` whose `blockingGet()` returns `null`
when empty; and it now says outright that `Maybe` is not yet recognised as a
data service return type, so the follow-up you suggest is visible.
- **`rxjava.adoc:93`** — names both plugins.
## On #16621
The plan is both in 9.0, this PR first and the removal as a follow-up on
`9.0.x` — which is what #16621 itself proposes, since the RxJava 3 port is what
keeps the data services working after `RxEntity` goes.
That makes your concern a real one, so I demoted the `RxEntity`-specific
advertising. What's New is now "RxJava Data Services on RxJava 3": it leads
with the data services over any GORM datastore, and says in one sentence that
the `RxEntity` model is ported but has no datastore behind it and is proposed
for removal in 9.0. The `ObservableSource` composition is no longer sold as a
9.0 feature there. I kept it in the upgrade guide, where it's a migration note
for someone porting code rather than an advertisement, and added a line there
and in `asyncRxGorm.adoc` saying the removal is under discussion and that an
`RxEntity` application should plan to move to `GormEntity` with the data
services.
Rerun of your probes welcome — in particular the `insert()` one and the two
thread assertions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
