Skip to content

Fix FactoryFinder path resolution in Windows#1831

Merged
cshannon merged 1 commit intoapache:mainfrom
cshannon:windows-path-fix
Mar 26, 2026
Merged

Fix FactoryFinder path resolution in Windows#1831
cshannon merged 1 commit intoapache:mainfrom
cshannon:windows-path-fix

Conversation

@cshannon
Copy link
Contributor

@cshannon cshannon commented Mar 25, 2026

This update ensures forward slashes are alow used for classpath
resolution, even on all Windows versions. Also add extra validation
to prevent path separators (slashes) in factory key names. Lastly added
some more tests for validation

Closes #1830

@cshannon
Copy link
Contributor Author

@wilkinsona - This is currently on the main branch (6.3.0-SNAPSHOT) and can be backported to the other branches for 6.2.3.

Do you have an easy way to test a snapshot to verify this fixes the issue? Interestingly it didn't show up in our Github actions Windows CI.

@cshannon cshannon requested review from jbonofre and jeanouii March 25, 2026 16:00
jbonofre
jbonofre previously approved these changes Mar 25, 2026
jbonofre
jbonofre previously approved these changes Mar 25, 2026
jeanouii
jeanouii previously approved these changes Mar 25, 2026
@wilkinsona
Copy link

Do you have an easy way to test a snapshot to verify this fixes the issue? Interestingly it didn't show up in our Github actions Windows CI.

Unfortunately not. Through GitHub Actions is the only way that I have to run tests on Windows so I'd have to set up a special CI build with access to ActiveMQ snapshots.

@cshannon
Copy link
Contributor Author

cshannon commented Mar 25, 2026

I need to make a tweak here as I realized by the startsWith() check that is done inside of resolvePath() should really be done after the final path is resolved (and any slashes are replaced) to make sure the actual final path that will be used gets validated.

Edit: Another possible simpler change would be just to prevent the key from containing any slashes, i'm running a couple tests now. I also am going to see if I can dig up an old Windows install to test manually before merging

This update ensures forward slashes are alow used for classpath
resolution, even on all Windows versions. Also add extra validation
to prevent path separators (slashes) in factory key names. Lastly added
some more tests for validation

This closes apache#1830
@cshannon cshannon dismissed stale reviews from jeanouii and jbonofre via ca7649e March 25, 2026 22:28
@cshannon cshannon requested a review from jbonofre March 25, 2026 22:29
@cshannon
Copy link
Contributor Author

@jbonofre - Ok assuming CI passes I think this is good, I added some extra tests for backward slashes and also validation for path separators in the name of the key as an extra check.

I think the root cause here has to do with differing versions of Windows and the JDK. I tested with Temurin JDK 25 on windows and everything works fine even with backslashes, so I'm guessing it depends on the exact environment. Replacing the backslashes should restore the previous behavior before we normalized and should hopefully fix the issue.

Once this is merged we can backport and create a 6.2.3 and 5.19.4 release

@snicoll
Copy link

snicoll commented Mar 26, 2026

@cshannon you can see the failures for our CI build. The upgrade fails on JDK 17, 21, 25, and 26 (Liberica) on Microsoft Windows Server 2025 (10.0.26100).

Perhaps the above would help you reproduce?

@cshannon
Copy link
Contributor Author

@cshannon you can see the failures for our CI build. The upgrade fails on JDK 17, 21, 25, and 26 (Liberica) on Microsoft Windows Server 2025 (10.0.26100).

Perhaps the above would help you reproduce?

@snicoll - Thanks for pointing that out, that was very helpful!

I cloned Spring Boot and ran the ActiveMQ tests against 6.2.2 with the same JDK and was able to reproduce the test failure as soon in the logs in your CI builds, so that was good news I could reproduce it. I then updated my local spring boot build to use a snapshot ActiveMQ build based on this PR and I confirmed the test now passes again and is fixed.

I will go ahead and merge this and we will get a 6.2.3 build out soon that will include this fix.

@tabish121 tabish121 self-requested a review March 26, 2026 13:50
Copy link
Contributor

@tabish121 tabish121 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, having any path elements feels like an invalid use so good to just block it outright.

@cshannon cshannon merged commit bedb108 into apache:main Mar 26, 2026
10 checks passed
@cshannon cshannon deleted the windows-path-fix branch March 26, 2026 14:11
cshannon added a commit to cshannon/activemq that referenced this pull request Mar 26, 2026
This update ensures forward slashes are alow used for classpath
resolution, even on all Windows versions. Also add extra validation
to prevent path separators (slashes) in factory key names. Lastly added
some more tests for validation

This closes apache#1830

(cherry picked from commit bedb108)
cshannon added a commit to cshannon/activemq that referenced this pull request Mar 26, 2026
This update ensures forward slashes are alow used for classpath
resolution, even on all Windows versions. Also add extra validation
to prevent path separators (slashes) in factory key names. Lastly added
some more tests for validation

This closes apache#1830

(cherry picked from commit bedb108)
cshannon added a commit that referenced this pull request Mar 26, 2026
This update ensures forward slashes are alow used for classpath
resolution, even on all Windows versions. Also add extra validation
to prevent path separators (slashes) in factory key names. Lastly added
some more tests for validation

This closes #1830

(cherry picked from commit bedb108)
cshannon added a commit that referenced this pull request Mar 26, 2026
This update ensures forward slashes are alow used for classpath
resolution, even on all Windows versions. Also add extra validation
to prevent path separators (slashes) in factory key names. Lastly added
some more tests for validation

This closes #1830

(cherry picked from commit bedb108)
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.

FactoryFinder regression on Windows

6 participants