-
Notifications
You must be signed in to change notification settings - Fork 279
Add some changes related to RDAP Feb 2024 profile #2759
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
Conversation
8c9ffba
to
2fed4ba
Compare
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.
Reviewed 6 of 63 files at r1, all commit messages.
Reviewable status: 6 of 63 files reviewed, 3 unresolved discussions
core/src/main/java/google/registry/rdap/RdapActionBase.java
line 291 at r1 (raw file):
jsonArray.forEach(this::addLinkValuesRecursively); } if (jsonElement instanceof JsonObject jsonObject) {
I think else if
would make it clearer that these are separate cases, even despite the fact that element can't be an instance of both.
core/src/main/java/google/registry/rdap/RdapActionBase.java
line 293 at r1 (raw file):
if (jsonElement instanceof JsonObject jsonObject) { if (jsonObject.has("links")) { addLinkValues(jsonObject.getAsJsonArray("links"));
Do you wanna check if that's actually an array first before getting it as array? https://www.javadoc.io/doc/com.google.code.gson/gson/2.6.2/com/google/gson/JsonElement.html#isJsonArray--
Code quote:
getAsJsonArray("links")
core/src/main/java/google/registry/rdap/RdapActionBase.java
line 300 at r1 (raw file):
private void addLinkValues(JsonArray linksArray) { linksArray.forEach(
nit, can we replace with steam() + filter() + forEach()
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.
Reviewable status: 6 of 63 files reviewed, 4 unresolved discussions (waiting on @gbrodman)
core/src/test/java/google/registry/rdap/RdapActionBaseTestCase.java
line 160 at r1 (raw file):
JsonObject createTosNotice() { return GSON.toJsonTree(
It seems odd, why not just write a json instead? These are basic types - strings mostly.
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.
Reviewable status: 5 of 63 files reviewed, 3 unresolved discussions (waiting on @ptkach)
core/src/main/java/google/registry/rdap/RdapActionBase.java
line 291 at r1 (raw file):
Previously, ptkach (Pavlo Tkach) wrote…
I think
else if
would make it clearer that these are separate cases, even despite the fact that element can't be an instance of both.
Done.
core/src/main/java/google/registry/rdap/RdapActionBase.java
line 293 at r1 (raw file):
Previously, ptkach (Pavlo Tkach) wrote…
Do you wanna check if that's actually an array first before getting it as array? https://www.javadoc.io/doc/com.google.code.gson/gson/2.6.2/com/google/gson/JsonElement.html#isJsonArray--
yeah fair, it always should be, but good to check
core/src/main/java/google/registry/rdap/RdapActionBase.java
line 300 at r1 (raw file):
Previously, ptkach (Pavlo Tkach) wrote…
nit, can we replace with steam() + filter() + forEach()
Done (requires a call to Streams)
core/src/test/java/google/registry/rdap/RdapActionBaseTestCase.java
line 160 at r1 (raw file):
Previously, ptkach (Pavlo Tkach) wrote…
It seems odd, why not just write a json instead? These are basic types - strings mostly.
this was copied over from the helper class and I kept the existing structure, but you're right that this can just be changed to a multiline string (sometimes with interpolation).
4436bfb
to
0e589fe
Compare
This implements two type of changes: 1. changing the link type for things like the terms of service 2. adding the request URL to each and every link with the "value" field. This is a bit tricky to implement because the links are generated in various places, but we can implement it by adding it to the results after generation. See b/418782147 for more information
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.
Reviewed 56 of 63 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gbrodman)
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.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @gbrodman)
This implements two type of changes:
See b/418782147 for more information
This change is