-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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.
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
Docs security JPA small enhancement and fix #37799
Conversation
🙈 The PR is closed and the preview is expired. |
Can you look at this @sberyozkin |
@@ -102,7 +103,7 @@ public class User extends PanacheEntity { | |||
@Password | |||
public String pass; | |||
|
|||
@ManyToMany | |||
@ManyToMany(cascade = CascadeType.ALL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jedla97 Does it have to be added here ? It seems to work without it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sberyozkin I look on this. Now I probably understand it more, mainly what this part should say. First I was just switch from roles defined as string (previous example) and use this. This caused the need for this. From my understanding is that this section showing that user roles can be stored as list.
Main point for this is that if we need to do some operation with user (add new role) it need the define cascade (at least I didn't find any way how to not use it). I'll update the PR as this don't need be part of the code, but add note as it can be useful for people reading this guide. If you won't any note about this please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jedla97 Thanks, yes, please add a note with some advice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Sorry for taking that long I had more important things on my list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Np at all, thanks @jedla97
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jedla97, LGTM, minor question is asked
dcc87d5
to
0e6b252
Compare
This PR fix links and adding some enhancement to security-jpa
Adding the summary is needed as build the docs fail without it.
For
ExternalRolesUserEntity
change I can replace the name ofUser
class to this. But probably I prefer this way as this can be extended from previous showcase.For
jpa.PasswordProvider
I think it better to split the classes to different snippets as it have two different import of password.Also if
org.wildfly.security.password.PasswordProvider
exist somewhere and should be used I would add which dependency because I didn't find it anywhere and it only used in Quarkus docs.