Move SinkEventAttributeSet to doxia-sink-api - #1073
Conversation
doxia-sink-api declares Sink methods taking a SinkEventAttributes but ships no implementation of that interface and no factory for one, so every caller that wants to pass attributes to a sink has to depend on doxia-core and import from the .impl package. The class was in package org.apache.maven.doxia.sink until DOXIA-506 moved it in with the sink implementations. Move it, along with its nested Semantics, next to the interface it implements. It brings no new dependencies with it: it needs only javax.swing.text.AttributeSet, JDK collections and SinkEventAttributes, and SinkEventAttributes already extends MutableAttributeSet. The old class stays behind as a deprecated subclass so that existing bytecode keeps working. It carries its own nested Semantics, which is not inherited under the old binary name, and overrides unmodifiable() to keep the old return type. The protected parser hooks in doxia-core that take this type keep taking the deprecated one, so nothing on the extension API changes; japicmp passes with no exclusions.
There was a problem hiding this comment.
Pull request overview
This PR relocates SinkEventAttributeSet (and its nested Semantics) into doxia-sink-api under org.apache.maven.doxia.sink, so consumers can construct SinkEventAttributes without depending on doxia-core internals, while retaining binary compatibility via a deprecated stub in doxia-core.
Changes:
- Add
org.apache.maven.doxia.sink.SinkEventAttributeSettodoxia-sink-apiand move its unit test alongside it. - Keep
org.apache.maven.doxia.sink.impl.SinkEventAttributeSetas a deprecated subclass for binary compatibility, and update imports across modules/tests to use the new public package. - Add
junit-jupiter-apias a test-scoped dependency todoxia-sink-api.
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| doxia-sink-api/src/main/java/org/apache/maven/doxia/sink/SinkEventAttributeSet.java | New public implementation moved into the Sink API module. |
| doxia-sink-api/src/test/java/org/apache/maven/doxia/sink/SinkEventAttributeSetTest.java | Test relocated to match the new public package. |
| doxia-sink-api/pom.xml | Adds JUnit Jupiter API as a test dependency for the moved test. |
| doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/SinkEventAttributeSet.java | Deprecated compatibility subclass retained in the old package. |
| doxia-core/src/main/java/org/apache/maven/doxia/macro/AbstractMacro.java | Switches imports to the new public SinkEventAttributeSet. |
| doxia-core/src/main/java/org/apache/maven/doxia/macro/snippet/SnippetMacro.java | Switches imports to the new public SinkEventAttributeSet. |
| doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/SinkUtils.java | Switches imports to the new public SinkEventAttributeSet. |
| doxia-core/src/main/java/org/apache/maven/doxia/sink/impl/Xhtml5BaseSink.java | Switches imports to the new public SinkEventAttributeSet. |
| doxia-core/src/main/java/org/apache/maven/doxia/util/DoxiaUtils.java | Switches imports to the new public SinkEventAttributeSet. |
| doxia-core/src/test/java/org/apache/maven/doxia/macro/toc/TocMacroTest.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-core/src/test/java/org/apache/maven/doxia/parser/AbstractParserTest.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-core/src/test/java/org/apache/maven/doxia/parser/Xhtml5BaseParserTest.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-core/src/test/java/org/apache/maven/doxia/sink/impl/AbstractSinkTest.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-core/src/test/java/org/apache/maven/doxia/sink/impl/AbstractXmlSinkTest.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-core/src/test/java/org/apache/maven/doxia/sink/impl/SinkAdapterTest.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-core/src/test/java/org/apache/maven/doxia/sink/impl/SinkTestDocument.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-core/src/test/java/org/apache/maven/doxia/sink/impl/SinkUtilsTest.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-core/src/test/java/org/apache/maven/doxia/sink/impl/Xhtml5BaseSinkTest.java | Updates imports to the new public SinkEventAttributeSet and Semantics. |
| doxia-modules/doxia-module-apt/src/main/java/org/apache/maven/doxia/module/apt/AptParser.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-modules/doxia-module-apt/src/main/java/org/apache/maven/doxia/module/apt/AptSink.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-modules/doxia-module-apt/src/test/java/org/apache/maven/doxia/module/apt/AptParserTest.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-modules/doxia-module-fml/src/main/java/org/apache/maven/doxia/module/fml/FmlContentParser.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-modules/doxia-module-fml/src/main/java/org/apache/maven/doxia/module/fml/FmlParser.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-modules/doxia-module-markdown/src/main/java/org/apache/maven/doxia/module/markdown/MarkdownSink.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-modules/doxia-module-markdown/src/test/java/org/apache/maven/doxia/module/markdown/MarkdownParserTest.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-modules/doxia-module-markdown/src/test/java/org/apache/maven/doxia/module/markdown/MarkdownSinkTest.java | Updates imports to the new public SinkEventAttributeSet and Semantics. |
| doxia-modules/doxia-module-xdoc/src/main/java/org/apache/maven/doxia/module/xdoc/XdocParser.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-modules/doxia-module-xdoc/src/main/java/org/apache/maven/doxia/module/xdoc/XdocSink.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-modules/doxia-module-xdoc/src/test/java/org/apache/maven/doxia/module/xdoc/XdocParserTest.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-modules/doxia-module-xdoc/src/test/java/org/apache/maven/doxia/module/xdoc/XdocSinkTest.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-modules/doxia-module-xhtml5/src/main/java/org/apache/maven/doxia/module/xhtml5/Xhtml5Parser.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-modules/doxia-module-xhtml5/src/main/java/org/apache/maven/doxia/module/xhtml5/Xhtml5Sink.java | Updates imports to the new public SinkEventAttributeSet. |
| doxia-modules/doxia-module-xhtml5/src/test/java/org/apache/maven/doxia/module/xhtml5/Xhtml5SinkTest.java | Updates imports to the new public SinkEventAttributeSet. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /** | ||
| * Replace this AttributeSet by an unmodifiable view of itself. | ||
| * Any subsequent attempt to add, remove or modify the underlying mapping | ||
| * will result in an UnsupportedOperationException. | ||
| * {@inheritDoc} | ||
| * | ||
| * @return an unmodifiable view of this AttributeSet. | ||
| * @since 1.1.1 | ||
| * Overridden only to keep returning this type, so that code compiled against the old signature keeps | ||
| * resolving the method. | ||
| */ | ||
| @Override | ||
| public SinkEventAttributeSet unmodifiable() { | ||
| this.attribs = Collections.unmodifiableMap(attribs); | ||
| super.unmodifiable(); | ||
|
|
||
| return this; | ||
| } |
There was a problem hiding this comment.
Good catch, this was a real regression and fixed in 0efdcd4.
clone() built its result with an explicit new SinkEventAttributeSet(...) rather than super.clone(), so the deprecated subclass would indeed have cloned to the parent type. japicmp cannot see it because no signature changes.
Fixed at the root rather than by overriding in the stub: clone() now uses super.clone() and replaces the shared fields afterwards. Behaviour of the class itself is unchanged, and any subclass now clones correctly. Added SinkEventAttributeSetCompatibilityTest in doxia-core covering the three things the deprecated class has to keep doing that japicmp cannot express: reaching the constants, keeping the return type of unmodifiable(), and cloning to itself. It fails on the previous implementation with exactly the type mismatch you describe.
| @Override | ||
| public int hashCode() { | ||
| final int parentHash = (resolveParent == null ? 0 : resolveParent.hashCode()); | ||
|
|
||
| return attribs.hashCode() + parentHash; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object obj) { | ||
| if (this == obj) { | ||
| return true; | ||
| } | ||
|
|
||
| if (obj instanceof SinkEventAttributeSet) { | ||
| return isEqual((SinkEventAttributeSet) obj); | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Correct as an observation, but out of scope here: this is pre-existing code that the PR only moves, byte for byte. Changing equals or hashCode semantics inside a move commit would hide a behaviour change in a diff that otherwise has none.
For what it is worth, the impact is currently theoretical: resolveParent is never set anywhere in Doxia or in the report plugins that use this class, only in the class's own unit test. Happy to raise it separately if a maintainer wants it addressed.
clone() built its result with an explicit "new SinkEventAttributeSet", so the deprecated subclass in doxia-core would have cloned to the parent type instead of its own, and code casting the result back to the old type would have started failing with a ClassCastException. japicmp cannot see this, since no signature changes. Use super.clone() and replace the shared fields afterwards, which keeps the existing behaviour of the class and makes any subclass clone correctly. The added test covers the three things the deprecated class has to keep doing that japicmp cannot express: reaching the constants, keeping the return type of unmodifiable(), and cloning to itself.
Keeping the old class as a subclass means the constants, and every attribute set Doxia creates internally, are now instances of the new class only. Code that tests them with instanceof or casts them to the old class has to move; japicmp cannot see that, since no signature changes. The alternative would have been to redeclare the constants on the deprecated class, which keeps the old runtime type but breaks their identity across the two names. The tests pin down the choice made here so it cannot be reversed by accident, and the javadoc on both classes says which way it went. Also record the version the class moved in and the version the old one was deprecated in.
|
Pushed a890a94 and expanded the description after a closer look at what the subclass approach actually costs. Three things worth surfacing rather than leaving for review to find: The constants change runtime type. Since the static initialisers now run in the new class, "No exclusions" was wrong in my original description; doxia-core already carries one for The parser hooks make the deprecation unfulfillable in 2.x, since the class cannot be removed while they reference it. Filed #1074 for retyping them to Also added |
Draft implementation of #1072, to make the discussion concrete. Happy to drop it if you would rather solve this differently.
doxia-sink-apideclaresSinkmethods that take aSinkEventAttributes, but ships no implementation of that interface and no factory for one. The only implementation lives indoxia-coreunderorg.apache.maven.doxia.sink.impl, so every caller that wants to pass attributes to a sink has to depend ondoxia-coreand import from a package whose name says it is internal. That is what MSHARED-1364 / apache/maven-reporting-impl#184 is about, and it applies to most report renderers in the ecosystem, not just that one.The class was in package
org.apache.maven.doxia.sinkuntil DOXIA-506 moved it in with the genuinely internal sink implementations.What this does
Moves
SinkEventAttributeSetand its nestedSemanticstodoxia-sink-api, in packageorg.apache.maven.doxia.sink, next to the interface it implements. Its unit test moves with it, which is why the module gains ajunit-jupiter-apitest dependency; it still has no compile dependencies.The move costs the API module nothing. The class needs only
javax.swing.text.AttributeSet, JDK collections andSinkEventAttributes, andSinkEventAttributesalready extendsjavax.swing.text.MutableAttributeSet.Binary compatibility
japicmppasses on every module, and this adds no exclusion of its own (doxia-core already carries one, forSinkUtils#filterAttributes). Three things were needed to get there, and the last one is the part worth your attention.org.apache.maven.doxia.sink.impl.SinkEventAttributeSetstays as a deprecated subclass of the new class. The static constants resolve through it, since agetstaticresolves through superclasses.It carries its own deprecated nested
Semanticssubclass and an override ofunmodifiable(). Neither comes for free: a nested class is not inherited under its old binary name, so existing code referencing…/sink/impl/SinkEventAttributeSet$Semanticswould fail withNoClassDefFoundError, andunmodifiable()declares its own type as the return type, so the inherited one has the wrong descriptor.The protected parser hooks in
doxia-corethat take or return this type keep taking the deprecated one, so nothing on the extension API changes:AbstractXmlParser.getAttributesFromParser(XmlPullParser)AbstractXmlParser.handleUnknown(String, SinkEventAttributeSet, Sink, int)Xhtml5BaseParser.baseStartTag(String, SinkEventAttributeSet, Sink)Xhtml5BaseParser.baseEndTag(String, SinkEventAttributeSet, Sink)Xhtml5BaseParser.consecutiveSections(int, Sink, SinkEventAttributeSet)Retyping those would be a silent break rather than a loud one: a subclass compiled against the old signature would stop overriding the method and simply never be called again. So I left them alone. They arguably want to take the
SinkEventAttributesinterface rather than a concrete set at all, but that is a separate change needing a version bump, and it is not what the issue is about.What does change: the runtime type of attribute sets
Worth being explicit, because japicmp cannot see it and it is the one thing here that is not free. Because the old class is now a subclass, the constants and every attribute set Doxia creates internally are instances of the new class only:
Code that tests those values with
instanceof, or casts them to the old class, has to move to the new one. Reading the constants through either name still yields the very same objects.The alternative was to redeclare the constants on the deprecated class. That keeps the old runtime type, at the price of
impl.SinkEventAttributeSet.BOLD != SinkEventAttributeSet.BOLD. I picked identity over the old type, on the grounds that comparing and passing these constants around is common and downcasting them is not, but it is a judgement call and I am happy to flip it. Either way it is now pinned by tests rather than left implicit, and both classes say in their javadoc which way it went.Source compatibility is otherwise preserved, with one exception the japicmp setup does not cover (
onlyBinaryIncompatibleis true): a file wildcard-importing bothorg.apache.maven.doxia.sink.*andorg.apache.maven.doxia.sink.impl.*now has an ambiguousSinkEventAttributeSetand needs a single-type import.Why a subclass rather than a copy
MSHARED-1364 suggested copying the class and deprecating the old one. A subclass avoids two bodies of the same code drifting apart, and it is what keeps the constants identical across both names. The cost is that the new class can never become
final, and the newSemanticshas to keep an accessible constructor rather than becoming a proper uninstantiable constants holder, because the deprecated nested class extends it. Say the word if you would rather have the copy.Versioning
This adds a class to a published API module, so it wants 2.2.0 rather than 2.1.1. The javadoc is written accordingly.
Follow-up
The deprecated class cannot actually be removed while those parser hooks reference it, so #1074 tracks retyping them to
SinkEventAttributesin the next major and dropping the class after.Once this is released,
AbstractMavenReportRendererin maven-reporting-impl is a one-line import change and MSHARED-1364 is done.Verification
mvn verifyis green: full test suite, rat, and japicmp across all modules.Beyond the signature checking,
SinkEventAttributeSetCompatibilityTestin doxia-core pins the runtime behaviour of the deprecated class: the constants stay reachable and identical through it,unmodifiable()keeps returning this type,clone()clones to this type, and the constants are no longer instances of it. Theclone()case in particular fails against the previous implementation.