add test for issue #453 - fix is done in neko

[rbri]

use 4.2.0-SNAPSHOT

[rbri]

The check fails because the pom requires the entry for the snapshot repo.

see https://github.com/HtmlUnit/htmlunit?tab=readme-ov-file#maven-1 for more

[rbri]

@davewichers @spassarop sorry for the lazy comment, this is fixed with the latest neko snapshot

[spassarop]

LGTM. My only doubt is we should have the snapshots repo on the pom.xml or just wait for the 4.2.0 release.
I guess it is no problem but I am not used to work with Java public projects and snapshots. @davewichers is it ok? If it is it can be merged.

[kwwall]

Just an FYI, but in my experience with the last 3 companies I've worked for generally would not allow SNAPSHOT dependencies (including transitive ones) unless perhaps it was for an emergency patch situation (which is hardly the case here). Some of them would allow RC (release candidate) dependencies though. Maybe ask if that's at least a possibility.

On Sat, Jun 1, 2024, 6:25 PM Sebastián Passaro ***@***.***> wrote: LGTM. My only doubt is we should have the snapshots repo on the pom.xml or just wait for the 4.2.0 release. I guess it is no problem but I am not used to work with Java public projects and snapshots. @davewichers <https://github.com/davewichers> is it ok? If it is it can be merged. — Reply to this email directly, view it on GitHub <#454 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAO6PG4FP2FGUQZOTFDIRF3ZFJC5JAVCNFSM6AAAAABILCJPHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBTGYYTCOBVGI> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-kevin

[davewichers]

@spassarop - just use this MR to TEST, we will NOT merge the pom change as we won't use a snapshot dependency in a release. Please test this and let me know if it works and you are OK with everything. If you are, please ask @rbri to release a new version of his library so we can upgrade and do a new release of AntiSamy with the new version.

[rbri]

Just an FYI, but in my experience with the last 3 companies I've worked for
generally would not allow SNAPSHOT dependencies (including transitive ones)
unless perhaps it was for an emergency patch situation (which is hardly the
case here). Some of them would allow RC (release candidate) dependencies
though. Maybe ask if that's at least a possibility.

Hi @spassarop, @davewichers, @kwwall
maybe there is some misunderstanding... ;-)

The idea of this PR is NOT to force you to use the SNAPSHOT build in the release!

The idea is for projects having this kind of close dependencies (you need fixes in neko to fix issues in antisamy)

• make a fix in neko, include some tests that proves the fix
• build a snapshot version of neko
• make a PR for antisamy including a test that shows that this fixes the problem in antisamy
  • this has to point to the neko snapshot to pass the ci
• merge the PR and make a snapshot build of antisamy - you can deploy snapshot build depending on snapshot builds to sonatype
• ask the reporter to test if the (published) snapshot solvers the problem
• if yes coordinate release build of the dependent lib
• switch to the release build an proof that the antisamy test suite still passes
• publish an antisamy release depending on the neko release (sonatype does not accept release builds depending on snapshots)

[kwwall]

@rbri wrote:

Just an FYI, but in my experience with the last 3 companies I've worked for
generally would not allow SNAPSHOT dependencies (including transitive ones)
unless perhaps it was for an emergency patch situation (which is hardly the
case here). Some of them would allow RC (release candidate) dependencies
though. Maybe ask if that's at least a possibility.

Hi @spassarop, @davewichers, @kwwall maybe there is some misunderstanding... ;-)

The idea of this PR is NOT to force you to use the SNAPSHOT build in the release!

The idea is for projects having this kind of close dependencies (you need fixes in neko to fix issues in antisamy)
* make a fix in neko, include some tests that proves the fix
* build a snapshot version of neko

You've already done this part, right?

* make a PR for antisamy including a test that shows that this fixes the problem in antisamy
  * this has to point to the neko snapshot to pass the ci

For AntiSamy to use org.htmlunit:neko-htmlunit:4.2.0-SNAPSHOT, you still need to publish it to a Maven repo, correct?
Only instead of publishing it to OSSRH, you are apparently proposing to push it to the OSS Sonatype snapshots,
https://s01.oss.sonatype.org/content/repositories/snapshots/ ?

That's what I don't understand. If @jeetu22 or anyone else needs to test this against their proprietary corporate code, there is a fair chance that they will not be able to access this by default from their corporate firewalls.

And while this may not affect @jeetu22, it may affect others who wish to test it.

So rather than pushing out a SNAPSHOT to that Sonatype snapshot repo, why not mark this an RC1 release candidate and drop it in OSSRH as normal? That would allow AntiSamy to create a PR that uses org.htmlunit:neko-htmlunit:4.2.0-RC1 and result in a lot more developers being able to test it without jumping through hoops.

* merge the PR and make a snapshot build of antisamy - you can deploy snapshot build depending on snapshot builds to sonatype

And AntiSamy would release 1.7.6-RC1 in OSSRH rather than 1.7.6-SNAPSHOT in the OSS Sonatype snapshot repo. That doesn't seem like a lot of additional work to me, but more importantly it drastically lowers the bar to all Neko and AntiSamy clients who wish to test it. ESAPI has released several release candidates like this before. That's sort of how the process works. SNAPSHOT implies more like an ongoing, potentially very unstable development stream. RC# implies a succession of slightly more stable alpha or beta test releases. (Although, some OSS projects seem to use '-M#' suffixes for this, instead of 'RC'; I'm not sure what the 'M' stands for though.)

This also simplifies things from AntiSamy's proposal as they don't need to temporarily change the section and then remember to change it back in their pom.xml when they are ready to do the stable 1.7.6 release.

* ask the reporter to test if the (published) snapshot solvers the problem
* if yes coordinate release build of the dependent lib
* switch to the release build an proof that the antisamy test suite still passes
* publish an antisamy release depending on the neko release (sonatype does not accept release builds depending on snapshots)

I think we're on the same page EXCEPT, I'm advocating using Release Candidates instead of Snapshots so OSSRH can still be used. That's going to be a lot less hassle for all of the Neko and AntiSamy clients even it it potentially is a bit of additional extra re-work for you and the AntiSamy team. But IMO, it's the right thing to do.

Just my $.02. Neither of these are my project and I do all my ESAPI work from my personal laptop, so I'm not sitting behind a corporate firewall / filtering proxy. (Besides, this doesn't affect ESAPI, so there's no urgency of testing it before the official 1.7.6 AntiSamy release.) But some of your clients potentially will be negatively affected if SNAPSHOTs are used, and I think that's something to consider.

[spassarop]

Um, what Kevin says sounds reasonable. Again, Java public code management, can't give an opinion.

On the other side, the test is fine, I reviewed it. So you can go on with how you deem appropriate on the release :)

@rbri Thanks again for your efforts and taking the time to get involved in these kind of fixes and discussions even though it is not your own project, same for @kwwall ;)

[davewichers]

This all sounds way too complicated for me. I'll I do, and others can do this too, is pull the version of org.htmlunit:neko-htmlunit:4.2.0-SNAPSHOT locally, install the snapshot locally, and update their pom to use and test it locally. If everything looks good/works, then we are good. No need to push RC candidates or add then remove temporary pom changes. We had to do this before and its pretty easy for the reporter to do the same thing to test it themselves locally before we do an official release.

[kwwall]

@davewichers - I'm fine with this if you only want the reporter of issue #453 to confirm it is fixed and they know now to install things locally, which I assume is the case. But if the thought is to treat it more like an alpha or beta release with a few dozen people testing it, an RC update might make more sense. (However, that seems unlikely to be the case, so I'm fine with what you suggest.)

[rbri]

will do a release today or tomorrow...

[davewichers]

@spassarop @rbri - I've updated the pom in the main branch to use neko-html 4.2.0. @rbri - can you update the PR to remove the pom change so the only changes are the additional test case(s) you've created. @spassarop - any other changes you'd like to see in the PR?

[rbri]

@davewichers done - hope leaving in the neko release 4.2.0 is ok

[davewichers]

@spassarop - I tested this PR and it passes with 4.2.0 and fails with 4.1.0 (which is as expected). If you are good with the test case @rbri wrote properly covering the reported issue, I'll merge.

[rbri]

sorry folks, i made a mistake during the release - please use version 4.2.1 (see HtmlUnit/htmlunit-neko#120)

[davewichers]

I upgraded in main. @rbri - can you upgrade this PR to match?

[rbri]

done

[davewichers]

@rbri Can you not introduce a property here? We only use it in one place, so our style is to not create properties that are only used once.

[rbri]

Yes i can ;-)

Maybe you can squeeze on merge....

[davewichers]

Sure. I'll squash when we merge. @spassarop - are you good with the test case being sufficient so I can merge?

[spassarop]

I am

…

On Thu, 6 Jun 2024 at 11:39 Dave Wichers ***@***.***> wrote: Sure. I'll squash when we merge. @spassarop <https://github.com/spassarop> - are you good with the test case being sufficient so I can merge? — Reply to this email directly, view it on GitHub <#454 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHL3BMMHSGM3QP2QDLFDADTZGBYATAVCNFSM6AAAAABILCJPHCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJSG4YDONBSGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>