[MNG-8749] Remove PathTranslator and UrlNormalizer from public API - #12941
Conversation
gnodet
left a comment
There was a problem hiding this comment.
I don't think "single implementation" is a valid criterion for removing an SPI here. If it were, we'd need to remove nearly the entire services/model package — 14 out of 18 interfaces there have exactly one implementation:
DependencyManagementImporter, DependencyManagementInjector, InheritanceAssembler, ModelInterpolator, ModelNormalizer, ModelPathTranslator, ModelUrlNormalizer, ModelValidator, PluginConfigurationExpander, PluginManagementInjector, ProfileInjector, ProfileSelector, RootLocator, UrlNormalizer — all have a single impl.
That's not a design smell — that's the design. This package is an injection-based extension mechanism: each interface has one default implementation, and the interface exists so that extensions can swap in alternatives via DI. As I mentioned in the issue, plugins and extensions do customize model building by replacing individual components. Removing the interface and hardcoding DefaultPathTranslator in constructor signatures closes that extensibility point.
The cost of keeping a 1-method interface is effectively zero. The cost of removing it and needing to reintroduce it later is a breaking change for anyone who coded against the current shape.
|
I think this approach is a large heap of YAGNI. This is just the one example I happened to stumble in first. Removing it from the public API makes the API and system simpler and still allows it to be added in later if it anyone ever needs, though I very much doubt anyone ever will. |
|
I did some research into what plugins and extensions in the Maven ecosystem actually use from the The package splits into three tiers based on real-world external usage: Tier 1 — Actively reimplemented by plugins (must keep as SPI) These have multiple independent custom implementations across the ecosystem (Maven 3 API today, which is the evidence base for what Maven 4 will need):
Tier 2 — Required by Tier 1 contracts or deliberately extensible Not reimplemented themselves, but needed by the interfaces that are:
Tier 3 — No external usage, safe to remove Zero reimplementations, not part of any extension contract:
Both are pure stateless functions — no fields, no dependencies, no side effects. The entire Tier 3 SPI surface is two utility methods. Rather than replacing interface-based DI with concrete-class-based DI (which keeps injecting a stateless singleton through 4+ constructors), a cleaner approach would be to make both static methods — eliminating the interface, the DI plumbing, and the constructor parameters from the 6 consumers altogether. |
|
That makes sense. Static methods are good for pure functions like these. Not every function needs to be turned into a pluggable object. Some should just work. |
org.apache.maven.api.services.model.PathTranslator has exactly one implementation that is only used internally. Remove it from the public API and fold its single alignToBaseDirectory method into the internal DefaultPathTranslator implementation. Closes #10326
Both interfaces are stateless pure functions with zero external reimplementations. Instead of replacing interface-based DI with concrete-class DI (the previous commit's approach), convert both to static utility methods — eliminating the interfaces, the DI plumbing, and the constructor parameters from all consumers. - Delete PathTranslator interface (already done) - Delete UrlNormalizer interface - Make DefaultPathTranslator.alignToBaseDirectory static - Make DefaultUrlNormalizer.normalize static - Remove constructor parameters from DefaultModelBuilder, DefaultModelInterpolator, DefaultModelPathTranslator, DefaultModelUrlNormalizer, and DefaultProfileActivationContext Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4f52772 to
010595d
Compare
gnodet
left a comment
There was a problem hiding this comment.
Clean removal of two pure-function SPI interfaces (PathTranslator, UrlNormalizer) from the public API, converting them to static methods on final utility classes. Well-motivated — both are stateless functions with no fields, no dependencies, no side effects, and zero external reimplementations (confirmed by gnodet's Tier 1/2/3 analysis). Eliminates unnecessary DI boilerplate across 6+ constructor chains.
Observations (informational):
- The compat layer is unaffected —
org.apache.maven.project.path.PathTranslatorandorg.apache.maven.model.path.UrlNormalizerare separate deprecated interfaces in different packages. - Three incidental changes from the rebase commit (unused
IOExceptionimport removal, blank line cleanup, testthrowsclause) are harmless but ideally would be in a separate commit for cleaner history. - Since 4.0.0 is still in RC, this API removal is appropriate timing. The
priority:blockeron the JIRA issue aligns with the rc-7 milestone target.
📋 PR Metadata
| Aspect | Current | Suggested |
|---|---|---|
| Labels | (none) | removed |
🤖 This review was generated by ForgeBot.
|
@elharo Please assign appropriate label to PR according to the type of change. |
Fixes #10326 / MNG-8749.
Summary
PathTranslatorandUrlNormalizerinorg.apache.maven.api.services.modelare pure stateless functions with exactly one implementation each, no external reimplementations, and no reason to be injectable. This PR removes both interfaces from the public API and converts their implementations to static utility methods.Changes
Interfaces removed
api/maven-api-spi/.../services/model/PathTranslator.java— deletedapi/maven-api-spi/.../services/model/UrlNormalizer.java— deletedImplementations converted to static utility classes
DefaultPathTranslator—finalclass withprivateconstructor;alignToBaseDirectory(String, Path)is now a static methodDefaultUrlNormalizer—finalclass withprivateconstructor;normalize(String)is now a static methodBoth classes had their
@Named,@Singleton, and@Injectannotations removed since they no longer participate in DI.Internal consumers updated (6 classes)
DefaultModelInterpolator— constructor reduced from 4 params to 2 (RootLocator,Interpolator); uses static callsDefaultModelPathTranslator— removedPathTranslatorfield and constructor param; uses static callDefaultModelUrlNormalizer— removedUrlNormalizerfield and constructor param; uses static callDefaultModelBuilder— removedPathTranslatorfield and constructor paramDefaultProfileActivationContext— removedPathTranslatorfrom all constructors; uses static callmaven-testingstubs (MojoExtension,RepositorySystemSupplier) — simplified constructor wiringRationale
Analysis of the 22 interfaces in
org.apache.maven.api.services.modelshows they fall into three tiers:ModelProcessor,ProfileActivator,ProfileSelector,ModelValidator,ModelInterpolatorProfileActivationContext,RootDetector,ModelVersionParser,ModelPathTranslator,ModelUrlNormalizer,LifecycleBindingsInjector,PluginConfigurationExpander,ModelTransformer,ModelParser,ModelNormalizer,DependencyManagementImporter,DependencyManagementInjector,InheritanceAssembler,PluginManagementInjector,ProfileInjectorPathTranslator,UrlNormalizerThis PR removes Tier 3 — the two interfaces that provide no extension-point value and are better expressed as static methods.