Add console DUM download

[ptkach]

This change is 

[ptkach]

@gbrodman This one is ready

Reviewable status: 0 of 12 files reviewed, all discussions resolved (waiting on @gbrodman)

[gbrodman]

Reviewed 3 of 9 files at r1, 8 of 9 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @ptkach)

---

console-webapp/src/app/domains/domainList.component.html line 19 at r3 (raw file):

      mat-stroked-button
      color="primary"
      href="/console-api/dum-download?registrarId={{

Just checking, does this work the way that I'd expect (where it just downloads the file)? I know sometimes broswers can be weird about downloading the file vs opening it in a window

---

core/src/main/java/google/registry/module/RequestComponent.java line 193 at r3 (raw file):

  ConsoleUserDataAction consoleUserDataAction();

  ConsoleDUMDownloadAction consoleDUMDownloadAction();

this is a bit nitty, but the style guide treats abbreviations as if they were just a standard word, e.g. GenericXmlSyntaxException

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 84 at r3 (raw file):

    consoleApiParams
        .response()
        .setHeader("Content-Disposition", "attachment; filename=Google_Registry_DUMS.csv");

in case we have support agents doing this across multiple registrars, we should include the registrar name (and possibly the date?) in the file I think

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 91 at r3 (raw file):

        .response()
        .setDateHeader(
            "Expires", DateTime.now(UTC).withTimeAtStartOfDay().plusDays(1).withTimeAtStartOfDay());

i don't think we need the multiple calls to withTimeAtStartOfDay?

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 101 at r3 (raw file):

      logger.atWarning().withCause(e).log(
          String.format("Failed to create DUM csv for %s", registrarId));
      if (writer != null) {

it should be not super difficult to use a try-with-resources to auto-close the writer no matter what

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 116 at r3 (raw file):

    String sql =
        SQL_TEMPLATE
            .replace(":registrarId", registrarId)

Use the entity manager's variable replacement instead of this to avoid possible weird things happening (we've already verified that they have the permission so we shouldn't have a weird registrar name, but just in case)

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 130 at r3 (raw file):

    ImmutableList<String[]> formattedRecords =
        queryResult.stream().map(r -> r.split(",")).collect(toImmutableList());

Have you tested the performance of the entity manager's getResultStream vs getResultList? An alternative to this code would be using the result stream. This would mean we'd be constructing one fewer giant list, but we'd have to do this stream / map (formatting) inside the transaction

---

core/src/test/java/google/registry/testing/FakeResponse.java line 45 at r3 (raw file):

  private String lastResponseStackTrace;

  public final StringWriter writer = new StringWriter();

these can be private like the others, just using the getter?

---

core/src/test/java/google/registry/ui/server/console/ConsoleDUMDownloadActionTest.java line 85 at r3 (raw file):

    FakeResponse response = (FakeResponse) consoleApiParams.response();
    assertThat(response.getStatus()).isEqualTo(HttpStatusCodes.STATUS_CODE_OK);
    ImmutableList<String> actual = ImmutableList.copyOf(response.writer.toString().split("\r\n"));

hold up, \r\n? that seems wrong, we probably want just \n

---

core/src/test/java/google/registry/ui/server/console/ConsoleDUMDownloadActionTest.java line 104 at r3 (raw file):

  }

  ConsoleDUMDownloadAction createAction(Optional<AuthResult> maybeAuthResult) {

nit, this method can be private

[ptkach]

Reviewable status: 5 of 12 files reviewed, 10 unresolved discussions (waiting on @gbrodman and @Github-advanced-security[bot])

---

console-webapp/src/app/domains/domainList.component.html line 19 at r3 (raw file):

Previously, gbrodman wrote…

Just checking, does this work the way that I'd expect (where it just downloads the file)? I know sometimes broswers can be weird about downloading the file vs opening it in a window

Yes, just click the link get the file

---

core/src/main/java/google/registry/module/RequestComponent.java line 193 at r3 (raw file):

Previously, gbrodman wrote…

this is a bit nitty, but the style guide treats abbreviations as if they were just a standard word, e.g. GenericXmlSyntaxException

Done.

---

core/src/test/java/google/registry/testing/FakeResponse.java line 45 at r3 (raw file):

Previously, gbrodman wrote…

these can be private like the others, just using the getter?

Done.

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 84 at r3 (raw file):

Previously, gbrodman wrote…

in case we have support agents doing this across multiple registrars, we should include the registrar name (and possibly the date?) in the file I think

I've considered it, but decided against for multiple reasons:

• I think the target audience for this feature is actually registrars, not the agents. Having this ability to download DUMs in the console without reaching out to registry tech support is what we are aiming for here for partners.
• Performance-wise registrar name in the file will require a call to DB. This seems like something we probably want to avoid in otherwise "heavy" pathway.
• My understanding is that this is not to be used frequently by registrars, like monthly or so, so date can be omitted based on that
• Name is quite long as it is, not sure adding more text to it would make it more user friendly

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 91 at r3 (raw file):

Previously, gbrodman wrote…

i don't think we need the multiple calls to withTimeAtStartOfDay?

Oops, not sure how did it end up there twice :)

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 101 at r3 (raw file):

Previously, gbrodman wrote…

it should be not super difficult to use a try-with-resources to auto-close the writer no matter what

Done.

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 116 at r3 (raw file):

Previously, gbrodman wrote…

Use the entity manager's variable replacement instead of this to avoid possible weird things happening (we've already verified that they have the permission so we shouldn't have a weird registrar name, but just in case)

Updated to setParameter. For my understanding - what weird things have you anticipated?

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 130 at r3 (raw file):

Previously, gbrodman wrote…

Have you tested the performance of the entity manager's getResultStream vs getResultList? An alternative to this code would be using the result stream. This would mean we'd be constructing one fewer giant list, but we'd have to do this stream / map (formatting) inside the transaction

yes, getResultStream was my first attempt at it, but this currently is the most performant solution.

---

core/src/test/java/google/registry/ui/server/console/ConsoleDUMDownloadActionTest.java line 29 at r2 (raw file):

Previously, github-advanced-security[bot] wrote…

Unused classes and interfaces

Unused class: ConsoleDUMDownloadActionTest is not referenced within this codebase. If not used as an external API it should be removed.

Show more details

Done.

---

core/src/test/java/google/registry/ui/server/console/ConsoleDUMDownloadActionTest.java line 85 at r3 (raw file):

Previously, gbrodman wrote…

hold up, \r\n? that seems wrong, we probably want just \n

Can you elaborate on why is it wrong? This is a default CSV format used by Writer based on the https://www.ietf.org/rfc/rfc4180.txt

[gbrodman]

Reviewed 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Github-advanced-security[bot] and @ptkach)

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 84 at r3 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

I've considered it, but decided against for multiple reasons:

• I think the target audience for this feature is actually registrars, not the agents. Having this ability to download DUMs in the console without reaching out to registry tech support is what we are aiming for here for partners.
• Performance-wise registrar name in the file will require a call to DB. This seems like something we probably want to avoid in otherwise "heavy" pathway.
• My understanding is that this is not to be used frequently by registrars, like monthly or so, so date can be omitted based on that
• Name is quite long as it is, not sure adding more text to it would make it more user friendly

Those are fair (though I will note that #2 is not true since the registrar ID is passed in, but that's moot)

One thing I will note is that we probably shouldn't hardcode Google Registry but rather we should use config.registryPolicy.productName in some way

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 116 at r3 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

Updated to setParameter. For my understanding - what weird things have you anticipated?

Mainly just worried about SQL injection. Like I said that wouldn't be a huge issue as the code is currently structured since we verify the registrar first, but it seems like good practice regardless

---

core/src/test/java/google/registry/ui/server/console/ConsoleDUMDownloadActionTest.java line 85 at r3 (raw file):

Previously, ptkach (Pavlo Tkach) wrote…

Can you elaborate on why is it wrong? This is a default CSV format used by Writer based on the https://www.ietf.org/rfc/rfc4180.txt

oh dang I didn't know it was standard, I just assumed that we'd want to use only \n due to linux/osx standards.

[gbrodman]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Github-advanced-security[bot] and @ptkach)

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 84 at r3 (raw file):

Previously, gbrodman wrote…

Those are fair (though I will note that #2 is not true since the registrar ID is passed in, but that's moot)

One thing I will note is that we probably shouldn't hardcode Google Registry but rather we should use config.registryPolicy.productName in some way

not sure what happened with that link, but i was referring to point number 2

[ptkach]

Reviewable status: 10 of 15 files reviewed, 3 unresolved discussions (waiting on @gbrodman and @Github-advanced-security[bot])

---

core/src/main/java/google/registry/ui/server/console/ConsoleDUMDownloadAction.java line 84 at r3 (raw file):
Added the name to the config.

is not true since the registrar ID is passed in, but that's moot)
Well registrarId indeed is passed in the params, but registrar name isn't, getting the name would require a DB call

---

core/src/test/java/google/registry/ui/server/console/ConsoleDumDownloadActionTest.java line 44 at r4 (raw file):

Previously, github-advanced-security[bot] wrote…

Unused classes and interfaces

Unused class: ConsoleDumDownloadActionTest is not referenced within this codebase. If not used as an external API it should be removed.

Show more details

Done.

[gbrodman]

Reviewed 5 of 5 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Github-advanced-security[bot])