Skip to content

ConversionService should be configurable for BasicLookupStrategy and JdbcAclService #4819

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Oct 31, 2018
Merged

ConversionService should be configurable for BasicLookupStrategy and JdbcAclService #4819

merged 2 commits into from
Oct 31, 2018

Conversation

pwheel
Copy link
Contributor

@pwheel pwheel commented Nov 13, 2017 •
edited by jzheaux
Loading

AclClassIdUtils was added in #4424 but should be made public so can be instantiated via Java Config expose its ConversionService so that BasicLookupStrategy and JdbcAclService can configure it.

Fixes #4814

@pwheel pwheel force-pushed the gh-4814-AclClassIdUtils-public branch from 4e5bb88 to 7714973 Compare March 20, 2018 23:18
@pwheel
Copy link
Contributor Author

pwheel commented Mar 21, 2018

Hi @rwinch can you take a look at this please? Minor fix to enable Java Config to be used.

Thanks,
Paul

@rwinch
Copy link
Member

rwinch commented Mar 21, 2018

Thanks for the PR and the ping.

I think perhaps a better way of handling this is to allow injecting the ConversionService into classes using AclClassIdUtils and then internally the setter method creates a AclClassIdUtils instance with the ConversionService.

@rwinch rwinch added the status: waiting-for-feedback We need additional information before we can continue label Mar 21, 2018
@pwheel pwheel force-pushed the gh-4814-AclClassIdUtils-public branch from 7714973 to 8efec02 Compare March 25, 2018 21:59
@pwheel
Copy link
Contributor Author

pwheel commented Mar 25, 2018

Yep, that sounds good @rwinch, thanks. I was never quite happy with this bit, I like your suggestion.

I've made the changes.
Cheers.

@pwheel
Copy link
Contributor Author

pwheel commented Apr 7, 2018

@rwinch ping :-)

@pwheel
Copy link
Contributor Author

pwheel commented May 10, 2018

Hi @rwinch are you able to give some feedback on this? I've made some changes in keeping with your suggestions I hope.

Thanks,
Paul

@rwinch rwinch added in: acl An issue in spring-security-acl and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 31, 2018
@rwinch rwinch self-assigned this Oct 31, 2018
@rwinch rwinch added this to the 5.2.0.M1 milestone Oct 31, 2018
@rwinch rwinch added the type: enhancement A general enhancement label Oct 31, 2018
@rwinch rwinch merged commit ccc4e1c into spring-projects:master Oct 31, 2018
@rwinch
Copy link
Member

rwinch commented Oct 31, 2018

Thanks for the PR & sorry for the delays @pwheel! This is now merged into master

@pwheel
Copy link
Contributor Author

pwheel commented Nov 7, 2018

No worries @rwinch I should have chased you harder! 😄 That's awesome, thanks a lot

@rwinch
Copy link
Member

rwinch commented Nov 7, 2018

It was all me. I need to be better about being responsive (I'm focusing on that going forward) Thanks again for the PR!

@pwheel pwheel mentioned this pull request Nov 13, 2019
Closed
@jzheaux jzheaux changed the title AclClassIdUtils should be public ConversionService should be configurable for BasicLookupStrategy and JdbcAclService Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@rwinch rwinch

Labels
in: acl An issue in spring-security-acl type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.0.M1
Development

Successfully merging this pull request may close these issues.

ConversionService should be configurable for BasicLookupStrategy and JdbcAclService
2 participants
@pwheel @rwinch