gnodet commented on PR #13276:
URL: https://github.com/apache/maven/pull/13276#issuecomment-5865425772
Thanks for this — the use case is clearly stated and the implementation is
clean. I have a few concerns about the API addition before this goes in.
## `buildRaw` encodes a limitation that belongs to the caller, not the API
The new `ModelBuilderSession.buildRaw()` is essentially "run the model
builder pipeline but collect problems instead of throwing on the first
validation error". That's a useful and legitimate contract. But the name
encodes *how far the pipeline runs* (raw = file + raw model phase only) as if
that were fixed by the method, when it's already controlled by `RequestType` on
the `ModelBuilderRequest`.
I'd suggest renaming it to `validate`:
```java
/**
* Validates the model described by the request, collecting all problems
* without throwing on validation errors. The request's {@link
ModelBuilderRequest.RequestType}
* controls how far the pipeline runs. Only throws when the model cannot be
read at all.
*
* @since 4.1.0
*/
default ModelBuilderResult validate(ModelBuilderRequest request) throws
ModelBuilderException {
...
}
```
Same implementation for now — the raw case stays as-is. But the name doesn't
bake in the limitation, and we don't need a second API addition later when
someone wants effective-model validation.
## Effective-model validation is already achievable — without any new API
The PR description notes that validation stops at the raw model, and that a
dependency whose version comes from the parent's `dependencyManagement` is only
caught in `validateEffectiveModel`. That's true today, but it doesn't require
any new API to fix. The `ModelResolver` SPI is the customization point: an
implementation that resolves parents (from the batch being validated, or from
Central into a temporary local repository) lets the model builder do full
effective-model validation without touching the caller's local repo. The use
case maps directly onto the `ApiRunner` customizer:
```java
ApiRunner.builder()
.customizer(container -> container.addBean(ModelResolver.class,
new CentralBackedResolver(tempLocalRepo)))
.build()
.run(session -> session.getService(ModelBuilder.class)
.createSession()
.validate(requestWithBuildProjectType));
```
`mappedSources` inside `DefaultModelBuilder` already does the reactor
equivalent of this. A `validate()` method that honours `RequestType` gives
callers a clean upgrade path from "raw only" to "effective model from batch"
without any further API changes.
## The `clearRequestScopedCache` in `buildRaw` is a code smell
The `finally` block in `buildRaw` explicitly clears the request-scoped cache
because the method knows it shouldn't share session state with a concurrent
`build()` call on the same `ModelBuilderSession`. But the right answer isn't
special-casing the cleanup: it's using a fresh `ModelBuilderSession` per
validation job, which is what `createSession()` is for. A `validate()` method
that documents "use a dedicated session" doesn't need this workaround at all —
each `createSession()` + `validate()` is naturally isolated and the cache
lifecycle is just the session lifecycle.
--
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]
