Repository navigation
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.
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 |
slachiewicz
added
enhancement
equals() already follows isEqual() and compares only own attributes. hashCode was folding in the resolve parent, so two equal sets could have different hash codes. Rebased onto master after apache#1073 moved these files into doxia-sink-api. Fixes apache#1075
equals() already follows isEqual() and compares only own attributes. hashCode was folding in the resolve parent, so two equal sets could have different hash codes. Rebased onto master after apache#1073 moved these files into doxia-sink-api. Fixes apache#1075
equals() already follows isEqual() and compares only own attributes. hashCode was folding in the resolve parent, so two equal sets could have different hash codes. Rebased onto master after #1073 moved these files into doxia-sink-api. Fixes #1075
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.