gtld functionality #70

MonkeyIsNull · 2024-06-17T14:48:47Z

Here is the first pass at a PR, it is ready for comments but not a merge. Here is why:

No tests. I have the test JSON files but I haven't written the tests yet.
We are displaying Registrar Addresses but in the examples they don't do that. We can rip that out but I wanted the functionality b/c I thought it was very odd that it wasn't in there and didn't want to have to do the work for it later if we needed it.
in Tech/Admin etc. every part of the address fields displays even if it doesn't have data. The examples always have data. I am unclear whether we should display empty fields or not.
Every role except for abuse is missing the contact email. How the heck did I miss this? 🫨
The files. error.rs. help.rs, types.rs. network.rs I thought we would originally need them. We may need them in the future? I don't know. I believe we could just remove them. They are NOPs.
In entity.rs we have a String utility, I should move it to somewhere else. It looks weird just sitting there by itself. But where to move it? The other string util is in the md directory, and it doesn't really fit. Do we create our own string.rs for one util? Stick in mod.rs? 🤷

Code Organization. I'm not sure what you envisioned for how the code is broken out and utilized. We can certainly change it however you want.

MonkeyIsNull · 2024-06-17T19:29:06Z

Also, I had forgot to mention that the phone and the fax numbers were missing for roles. I have that working now but is not in this PR. Yet.

anewton1998

Overall looks good. Big thing is to use the Contact API, and if it doesn't support the data necessary to add whatever feature is needed.

I'll cover the other items from the bullet points in the PR convo.

anewton1998 · 2024-06-17T18:44:24Z

icann-rdap-cli/src/main.rs

@@ -292,6 +292,9 @@ enum OtypeArg {
    /// RDAP JSON with extra information.
    JsonExtra,

+    /// Global Top Level Domain Output
+    Gtld,


I'd prefer this be GtldWhois, which will make the option "gtld-whois" (I think)

anewton1998 · 2024-06-17T18:47:00Z

icann-rdap-client/src/gtld/mod.rs

+    }
+}
+
+pub trait ToGtld {


I know it is a bit wordy, but I think this should be "ToGtldWhois" for code readability.

anewton1998 · 2024-06-17T18:47:17Z

icann-rdap-client/src/gtld/mod.rs

+}
+
+pub trait ToGtld {
+    fn to_gtld(&self, params: &mut GtldParams) -> String;


as with above, "to_gtld_whois"

anewton1998 · 2024-06-17T18:49:54Z

icann-rdap-cli/src/query.rs

@@ -33,6 +34,9 @@ pub(crate) enum OutputType {
    /// Results are output as Pretty RDAP JSON.
    PrettyJson,

+    /// Global Top Level Domain Output
+    Gtld,


Though this is a different enum, it should also be "GtldWhois" to avoid confusion with other things that might be gTLD related in the future.

anewton1998 · 2024-06-17T19:03:26Z

icann-rdap-client/src/gtld/entity.rs

+}
+
+// Where do we move this to?
+// capitalize first letter


Unless you start collecting a lot of these string utilities, just leave it here. There is a strings.rs under md but that's because it required a lot of special cases.

anewton1998 · 2024-06-17T19:12:28Z

icann-rdap-client/src/gtld/entity.rs

+                    for role in roles {
+                        match role.as_str() {
+                            "registrar" => {
+                                if let Some(vcard_array) = &entity.vcard_array {


I think it would be more future proof if you used the Contact API with from_vcard (

icann-rdap/icann-rdap-common/src/contact/from_vcard.rs

Line 6 in 19de4d9

pub fn from_vcard(vcard_array: &[Value]) -> Option<Contact> {

) and add any functionality to Contact that is missing.

anewton1998 · 2024-06-17T19:22:50Z

icann-rdap-client/src/test_files/nic.xyz.json

+        "information pertaining to Internet domain names registered by our",
+        "our customers. By using this service you are agreeing (1) not to use any",
+        "information presented here for any purpose other than determining",
+        "ownership of domain names, (2) not to store or reproduce this data in",


This is gonna be problematic. Can you change this test file where the data isn't from centralnic but has the same structure?

The JSON files are for the tests, which I just finished. It looks like we can't use them so I will remove nic.xyz

anewton1998 · 2024-06-17T19:23:51Z

icann-rdap-client/src/test_files/nic.xyz.json

+            ]
+          ],
+          [
+            "ISO-3166-1-alpha-2",


If you do change this file, this is not the proper way to do this. You can remove this array.

anewton1998 · 2024-06-17T19:25:05Z

icann-rdap-client/src/test_files/nic.xyz.json

+          ],
+          [
+            "adr",
+            {},


to represent the country code, this JSON object should be "cc":"US".
See https://anewton1998.github.io/rdap_guide/protocol/jcard_and_vcard.html#adr-property

anewton1998 · 2024-06-17T19:28:48Z

icann-rdap-client/src/gtld/entity.rs

+            if let Some(roles) = &entity.roles {
+                for role in roles {
+                    if role.as_str() == "abuse" {
+                        if let Some(vcard_array) = &entity.vcard_array {


another comment about using the Contact API

anewton1998 · 2024-06-17T19:37:08Z

Addressing the bullet points:

No tests. I have the test JSON files but I haven't written the tests yet.

I believe in tests.

We are displaying Registrar Addresses but in the examples they don't do that. We can rip that out but I wanted the functionality b/c I thought it was very odd that it wasn't in there and didn't want to have to do the work for it later if we needed it.

Leave it. Seems useful.

in Tech/Admin etc. every part of the address fields displays even if it doesn't have data. The examples always have data. I am unclear whether we should display empty fields or not.

If we ever get redacted right, we'll probably display "REDACTED" or something. So just leave it, IMHO.

Every role except for abuse is missing the contact email. How the heck did I miss this? 🫨

There are a lot of tedious things in this.

The files. error.rs. help.rs, types.rs. network.rs I thought we would originally need them. We may need them in the future? I don't know. I believe we could just remove them. They are NOPs.

I vote to remove them if the have no value.

In entity.rs we have a String utility, I should move it to somewhere else. It looks weird just sitting there by itself. But where to move it? The other string util is in the md directory, and it doesn't really fit. Do we create our own string.rs for one util? Stick in mod.rs? 🤷

I commented in the code review, but with just one helper function it is fine to leave it there. If a bunch of string related helper functions start building up, then we can consider a refactor. The great thing about software is that it is malleable.

… name to GtldWhois

MonkeyIsNull · 2024-06-18T19:14:33Z

Removed the CentailNiC test file, added some others, finished the tests, added Role emails and fax, used the Contact API and changed the name to GtldWhois.
The one remaining bit that's left hanging that I can think of it the functions (fn) are still all to_gtld not to_gtldwhois.
Which do you prefer?

anewton1998 · 2024-06-18T21:13:41Z

I think to_gtld_whois.

MonkeyIsNull · 2024-06-19T15:07:41Z

Done!

anewton1998 · 2024-06-20T13:58:53Z

icann-rdap-client/src/gtld/entity.rs

+        .and_then(|orgs| orgs.get(0).cloned())
+        .unwrap_or_default();
+
+    // Contact address and the URL do not parse correctly, use the vcard.


Can you explain this comment?

Which URL are we talking about? The update URL of the complaint form should be in a notice, not the vCard.

If you are looking for the unstructured address (that label in adr), that should be here: https://github.com/icann/icann-rdap/blob/main/icann-rdap-common/src/contact/mod.rs#L167

I can explain/show this on our call but for the Address the short explanation is that the way address is structured in JSON didn't work well with the from_vcard call to PostalAddress builder and the 3rd line of street address ends up in the city and everything is misaligned by one step iirc. The URL is for the Registrars URL did not end up in the contact data structure either. Why they are that way I don't know 🤷

gtld functionality

e63f218

anewton1998 requested changes Jun 17, 2024

View reviewed changes

added tests, added role emails, use contact API for role_info, change…

befeb14

… name to GtldWhois

changing to_gtld to to_gtld_whois

b46ff1c

anewton1998 requested changes Jun 20, 2024

View reviewed changes

added TODOs and removed incorrect Registrar URL

daa0168

anewton1998 merged commit 80dcdc8 into icann:dev Jun 20, 2024
2 checks passed

anewton1998 mentioned this pull request Sep 3, 2024

gtld whois output #1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gtld functionality #70

gtld functionality #70

MonkeyIsNull commented Jun 17, 2024

MonkeyIsNull commented Jun 17, 2024

anewton1998 left a comment

anewton1998 Jun 17, 2024

anewton1998 Jun 17, 2024

anewton1998 Jun 17, 2024

anewton1998 Jun 17, 2024

anewton1998 Jun 17, 2024

anewton1998 Jun 17, 2024

anewton1998 Jun 17, 2024

MonkeyIsNull Jun 17, 2024 •

edited

Loading

anewton1998 Jun 17, 2024

anewton1998 Jun 17, 2024

anewton1998 Jun 17, 2024

anewton1998 commented Jun 17, 2024

MonkeyIsNull commented Jun 18, 2024

anewton1998 commented Jun 18, 2024

MonkeyIsNull commented Jun 19, 2024

anewton1998 Jun 20, 2024

anewton1998 Jun 20, 2024

anewton1998 Jun 20, 2024

MonkeyIsNull Jun 20, 2024

gtld functionality #70

gtld functionality #70

Conversation

MonkeyIsNull commented Jun 17, 2024

MonkeyIsNull commented Jun 17, 2024

anewton1998 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonkeyIsNull Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anewton1998 commented Jun 17, 2024

MonkeyIsNull commented Jun 18, 2024

anewton1998 commented Jun 18, 2024

MonkeyIsNull commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MonkeyIsNull Jun 17, 2024 •

edited

Loading