Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Make order setting mandatory for Realm config #51195
Make order setting mandatory for Realm config #51195
Changes from 18 commits
65cf24c
d186b0a
82e08d2
71bfb3e
5370163
1132f5f
7fa20a3
3d724e8
adca1bf
0f60b22
3cb7a83
22399ce
0edef24
b439a49
9548e2f
a74482e
e0aa65b
994c0af
da8e7e3
7fbf061
472b2d6
64d0292
aad6d19
84cb684
4c61828
c264e31
43fffa3
0614cbc
fe13c71
ad7a1b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This last phrase doesn't make sense, I would avoid describing again what
order
does.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.
Will do thanks
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.
I would remove
altogether.
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.
Somehow missed this occurrence ... Will remove. Thanks
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.
I would've probably made a different choice of where to place this check because we pass
RealmConfig
s around in too many places. For example, this:InternalRealms.getBootstrapChecks
so the stacktrace for it is funky.SamlMetadataCommand
which is not nice, although it might not happen in practice too often.I would've probably removed the
order
andenabled
local vars and do the checks inRealms#initRealms
. But there might be subtle problems in that case as well.Nevertheless, this does the job, and looks OK, to me.
Nit: we use the reverse form for negative condition, i.e.
false == <function>
(unless we are in some few older files that use a different convention in which case we blend with the background).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.
I am also tempted to move the logic into
Reamls#initRealms
(see below) comments for details. But your examples show that by itself is not sufficient. Both of the two examples are not covered if the logic is just inRealms#initRealms
, while they do get covered if the logic is inRealmConfig
. I am now again unsure about what would be the better approach.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.
Maybe I've not made myself clear
I mean that there is a minor inconvenience that the validation that the
order
parameter is defined is performed inside a different code than the code that deals with realm construction.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.
Not to dwell on it too much, but just because we need this new constructor is a smell. In this case, the
order
local var is sometimes the value from thesettings
, and sometimes a different one passed by this constructor. I know this is only used in tests, but it's still not nice to have to deal with it (Integer.MAX_VALUE
).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.
The new constructor is mostly used by test code, but it is also used by production code, see Realms and ReservedRealm.
I also think adding a new constructor is not ideal due to some of the concerns you mention. It however seems to be a better choice comparing to the other choice, i.e. Add
order
into thesettings
parameter on caller side so that it can just keep using the existing constructor.This approach should work fine for tests. But in production code, the
settings
object is referenced in many other classes, e.g.Environment
,Realms
, etc. So when adding FileRealm (in Realms.java), if we modify thesettings
object to addorder
, we will need to do something like the follows:So the
newSettings
is now passed into RealmConfig which will store a reference to it. Now we have twoSettings
object across the application. They are mostly identical, only differs in theorder
parameter. And this is just FileRealm, the same applies to native realm, for which we will have a thirdsettings
. I personally think this is worse than adding a new constructor. That is why the new constructor is added so we can avoid manipulating the settings object.With above being said, I realise it is making an assumption that the logic for mandating the
order
setting resides in theRealmConfig
class. If we can move away from this assumption, like you suggested, enforce the logic inRealms.initRealms
, we can probably make the change set much smaller.Initially I thought about adding the logic into
Security
but that was too high level. So we decided to add it inRealmConfig
itself, where it is low level the logic is close to its natural owner. But maybe we need something in between.Realms
could be the right candidate especially it is where the order uniqueness is enforced.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.
My choice would also be to only inspect the
order
setting inRealms#initRealms
(where theorder
value duplication is checked). That's because theorder
is only really used in that place, specifying the initial order in the realm chain (which would then change as we reorder the chain during authentications), so it's not suitable as a final local variable.This way we would also avoid having to specify the
order
parameter for the "default" realms (file
andnative
) which is simply ignored anyways.Your approach works as well and I can't argue strongly against it, so this LGTM to me even though I would've approached it differently.