Skip to content

Conversation

@eugene-kamenev
Copy link
Contributor

No description provided.

@matrei
Copy link
Contributor

matrei commented Aug 12, 2025

Thanks for the PR @eugene-kamenev!
Can you describe what issue/issues the changes in this PR is trying to solve?

@eugene-kamenev
Copy link
Contributor Author

@matrei There are couple of fixes:

  • 93486be acl object identity should be dirty checkable (this fixes updates not being propagated to DB)
  • ff94338 - fixes fetching all records from the DB to filter them, using criteria instead
  • bf21b7f - fixes cases for equality check (sid) and acl order.

I am very sorry, do not have test cases for the same.

@jdaugherty
Copy link
Contributor

I updated this branch for the publish fixes

@matrei matrei changed the title 7.0.x fixes ACL Plugin 7.0.x fixes Aug 16, 2025
@jamesfredley jamesfredley moved this to In Progress in Apache Grails Aug 19, 2025
@jamesfredley jamesfredley added this to the grails:7.0.0-RC2 milestone Aug 19, 2025
@matrei
Copy link
Contributor

matrei commented Aug 19, 2025

@eugene-kamenev The changes in AclService to use the new toSid() method in AclSid seems to break the functional tests:
https://github.com/apache/grails-spring-security/actions/runs/16915254212/job/47927447650?pr=1154#step:5:3372

@eugene-kamenev
Copy link
Contributor Author

@eugene-kamenev The changes in AclService to use the new toSid() method in AclSid seems to break the functional tests: https://github.com/apache/grails-spring-security/actions/runs/16915254212/job/47927447650?pr=1154#step:5:3372

Hi @matrei Thanks for your review. I will do the changes today and test on our repo. Would be great if you could help with test cases because failure is not very clear. It cannot find a method. But method exists, right? I will check this, looks to me like some issue with test setup.

@matrei
Copy link
Contributor

matrei commented Aug 19, 2025

@eugene-kamenev Thanks! I’m curious why the domain classes from the ACL plugin are duplicated in the test applications. Is it common practice to override the AclClass domain? If so, I’m a bit concerned about introducing a contract that requires toSid(). That change would break existing implementations that provide their own AclClass.

Also, it seems there are other test failures now that we are over the first hurdle: https://github.com/apache/grails-spring-security/actions/runs/17064317367/job/48378152123#step:5:3411

Copy link
Contributor

@matrei matrei left a comment

Choose a reason for hiding this comment

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

Thanks, nice work @eugene-kamenev !
One more small cleanup improvement suggestion, if you would like change that.
Otherwise I'll merge this as it is.

@matrei matrei merged commit 4011ce6 into apache:7.0.x Aug 20, 2025
12 of 14 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Apache Grails Aug 20, 2025
@eugene-kamenev eugene-kamenev deleted the 7.0.x-fixes branch August 22, 2025 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants