Skip to content

Conversation

harrisric
Copy link

@harrisric harrisric commented Feb 14, 2025

Thanks for your contribution to Apache Commons! Your help is appreciated!

Before you push a pull request, review this list:

  • [/] Read the contribution guidelines for this project.
  • [/] Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • [/] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible but is a best-practice.
  • [/] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • [/] Each commit in the pull request should have a meaningful subject line and body. Note that commits might be squashed by a maintainer on merge.

Changed the dependency on commons-digester to use commons-digester3 instead.

I understand that this may be blocked for a later major version change similarly to #53
running japicmp:cmp yields the following output:

[ERROR] Failed to execute goal com.github.siom79.japicmp:japicmp-maven-plugin:0.23.1:cmp (default-cli) on project commons-validator: There is at least one incompatibility: org.apache.commons.digester.ObjectCreationFactory[org.apache.commons.digester.ObjectCreationFactory]:INTERFACE_REMOVED,org.apache.commons.validator.FormSetFactory:SUPERCLASS_REMOVED,org.apache.commons.validator.FormSetFactory:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE -> [Help 1]

@garydgregory
Copy link
Member

garydgregory commented Feb 14, 2025

Hello @harrisric
-1: Obviously, since the tests don't even pass 👎 😞
And yes, breaking binary compatibility within a minor release line is a no-go. Can you avoid breaking BC here?

@harrisric
Copy link
Author

Hi @garydgregory - thanks for the quick response.
I did run a full mvn clean verify and received a passing build (using Java 17 locally)
[WARNING] Tests run: 990, Failures: 0, Errors: 0, Skipped: 1
running just mvn results in the japicmp:cmp failure noted above.
Looking at the failed builds linked here I can't see any test failures - the build just seems to end at the buildnumber:3.2.1:create (default) @ commons-validator goal. Happy to look into any details you can see.

I can't immediately see a way of avoiding the incompatibility - the class FormSetFactory extends an abstract class from commons-digester
i.e. from
FormSetFactory extends org.apache.commons.digester.AbstractObjectCreationFactory
to
FormSetFactory extends org.apache.commons.digester3.AbstractObjectCreationFactory<FormSet>

@garydgregory
Copy link
Member

Hello @harrisric
If this PR breaks binary compatibility, then we should not accept it.

I also see a lot of stack traces like:

Feb 14, 2025 10:08:03 AM org.apache.commons.digester3.Digester error
SEVERE: Parse Error at line 92 column 103: Element type "set-next-rule" must be declared.
org.xml.sax.SAXParseException; systemId: file:/Users/garygregory/git/commons-validator/target/classes/org/apache/commons/validator/digester-rules.xml; lineNumber: 92; columnNumber: 103; Element type "set-next-rule" must be declared.
	at java.xml/com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.createSAXParseException(ErrorHandlerWrapper.java:204)
	at java.xml/com.sun.org.apache.xerces.internal.util.ErrorHandlerWrapper.error(ErrorHandlerWrapper.java:135)

So there are likely other changes that would be needed as well.

@harrisric
Copy link
Author

@garydgregory - quite correct, that is simply a change of the named schema in the rule set. All those stack-traces are now gone. A little odd that this didn't bubble up as a test failure.

@garydgregory
Copy link
Member

@harrisric
Do you plan on updating this PR to get green build or should it be closed?

@harrisric
Copy link
Author

harrisric commented Mar 17, 2025

Hi @garydgregory as noted earlier, it is not possible within the compatibility constraints of a minor version change to get a branch change which succeeds with the aim of replacing the old dependency, as part of that dependency is directly in the class hierarchy of one of the public classes (FormSetFactory).
Is there a mechanism for this PR to be considered for inclusion in a 2.x version of this library?
Otherwise as you suggest this PR can be closed.

@garydgregory
Copy link
Member

Hello @harrisric
You'll want to get an account on our Jira instance and create a ticket for the VALIDATOR project.

@harrisric
Copy link
Author

Raised https://issues.apache.org/jira/browse/VALIDATOR-499 - let me know anything else I need to do here or can help out with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants