Skip to content
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

Support premium list updating in Cloud SQL #422

Merged
merged 5 commits into from Jan 2, 2020

Conversation

@CydeWeys
Copy link
Member

CydeWeys commented Dec 19, 2019

This also removes the requirement to specify --also_cloud_sql in nomulus premium
list tooling, instead always persisting to Cloud SQL. It removes a non-USD
premium label in the global test premium list (the Cloud SQL schema doesn't
support a mix of currency units in a single premium list).


This change is Reviewable

This also removes the requirement to specify --also_cloud_sql in nomulus premium
list tooling, instead always persisting to Cloud SQL. It removes a non-USD
premium label in the global test premium list (the Cloud SQL schema doesn't
support a mix of currency units in a single premium list). And it adds a method
to PremiumListDao to grab the most recent version of a given list.
@CydeWeys CydeWeys requested a review from gbrodman Dec 19, 2019
Copy link
Member Author

CydeWeys left a comment

+reviewer:@gbrodman

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

Copy link
Collaborator

gbrodman left a comment

Reviewable status: 0 of 17 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @gbrodman)


core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java, line 61 at r1 (raw file):

  public void run() {
    try {
      saveToDatastore();

saveToDatastore() sets the response payload, but that might happen before the call to the SQL save. What happens if the SQL save fails?


core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java, line 83 at r1 (raw file):

  }

  static google.registry.schema.tld.PremiumList parseToPremiumList(String name, String inputData) {

we're overriding the instance variables name and inputData here.

I noticed the only place we're calling this outside of these actions is in one of the tests. If we want to keep that test call, should we make this method static instead, just to make it clear that we're not using the instance variables?


core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java, line 79 at r1 (raw file):

  @Test
  public void update_worksSuccessfully() {

Potentially test deletion / modification of existing values in the premium list?


util/src/main/java/google/registry/util/EmailMessage.java, line 59 at r1 (raw file):

    public abstract Builder setBody(String body);
    public abstract Builder setRecipients(Collection<InternetAddress> recipients);
    public abstract Builder setFrom(InternetAddress from);

what's with the newlines here? they're not bad, just curious

Copy link
Member Author

CydeWeys left a comment

Reviewable status: 0 of 17 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @gbrodman)


core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java, line 61 at r1 (raw file):

Previously, gbrodman wrote…

saveToDatastore() sets the response payload, but that might happen before the call to the SQL save. What happens if the SQL save fails?

The error gets logged but otherwise everything works fine.


core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java, line 83 at r1 (raw file):

Previously, gbrodman wrote…

we're overriding the instance variables name and inputData here.

I noticed the only place we're calling this outside of these actions is in one of the tests. If we want to keep that test call, should we make this method static instead, just to make it clear that we're not using the instance variables?

This method is now static? I'm not sure what you're suggesting?


util/src/main/java/google/registry/util/EmailMessage.java, line 59 at r1 (raw file):

Previously, gbrodman wrote…

what's with the newlines here? they're not bad, just curious

No idea, the code formatter presubmit insisted on them (!!!).

Copy link
Collaborator

gbrodman left a comment

Reviewable status: 0 of 17 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @gbrodman)


core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java, line 83 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

This method is now static? I'm not sure what you're suggesting?

s/instance/injected

sorry, my mistake about the staticness. Basically, it seems odd / bad that we have both injected variables and method variables named "name" and "inputData". Maybe this should only use the injected variables, maybe it should be moved elsewhere, or maybe all it requires is a rename, but it just seems kinda smelly.


util/src/main/java/google/registry/util/EmailMessage.java, line 59 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

No idea, the code formatter presubmit insisted on them (!!!).

sigh, yeah i think it tends to do that

Copy link
Collaborator

gbrodman left a comment

Reviewable status: 0 of 17 files reviewed, 4 unresolved discussions (waiting on @CydeWeys)


core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java, line 61 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

The error gets logged but otherwise everything works fine.

all right -- is this something that we're concerned about? Obviously the saveToDatastore() method won't be around forever (hopefully) and as long as Datastore is the source of truth I suppose we would want the response data to be based off what happens to Datastore.

Copy link
Member Author

CydeWeys left a comment

Reviewable status: 0 of 17 files reviewed, 4 unresolved discussions (waiting on @CydeWeys and @gbrodman)


core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java, line 61 at r1 (raw file):

Previously, gbrodman wrote…

all right -- is this something that we're concerned about? Obviously the saveToDatastore() method won't be around forever (hopefully) and as long as Datastore is the source of truth I suppose we would want the response data to be based off what happens to Datastore.

It's fine. Datastore is still the primary source of truth here. The next step after this will be to make Cloud SQL the source of truth, upon which any errors there get reflected to the response data.


util/src/main/java/google/registry/util/EmailMessage.java, line 59 at r1 (raw file):

Previously, gbrodman wrote…

sigh, yeah i think it tends to do that

Done.

Copy link
Member Author

CydeWeys left a comment

Reviewable status: 0 of 17 files reviewed, 2 unresolved discussions (waiting on @CydeWeys and @gbrodman)


core/src/main/java/google/registry/tools/server/CreateOrUpdatePremiumListAction.java, line 83 at r1 (raw file):

Previously, gbrodman wrote…

s/instance/injected

sorry, my mistake about the staticness. Basically, it seems odd / bad that we have both injected variables and method variables named "name" and "inputData". Maybe this should only use the injected variables, maybe it should be moved elsewhere, or maybe all it requires is a rename, but it just seems kinda smelly.

Moved to a separate PremiumListUtils class. We'll see if there's enough meat there to justify its existence apart from PremiumListDao.

Copy link
Collaborator

gbrodman left a comment

Reviewable status: 0 of 18 files reviewed, 3 unresolved discussions (waiting on @CydeWeys and @gbrodman)


core/src/main/java/google/registry/schema/tld/PremiumListUtils.java, line 35 at r2 (raw file):

public class PremiumListUtils {

  public static google.registry.schema.tld.PremiumList parseToPremiumList(

out of curiosity is the full qualification of PremiumList necessary here?

Copy link
Member Author

CydeWeys left a comment

Reviewable status: 0 of 18 files reviewed, 3 unresolved discussions (waiting on @CydeWeys and @gbrodman)


core/src/main/java/google/registry/schema/tld/PremiumListUtils.java, line 35 at r2 (raw file):

Previously, gbrodman wrote…

out of curiosity is the full qualification of PremiumList necessary here?

Nope it's not.

Copy link
Member Author

CydeWeys left a comment

Reviewable status: 0 of 18 files reviewed, 3 unresolved discussions (waiting on @gbrodman)


core/src/test/java/google/registry/schema/tld/PremiumListDaoTest.java, line 79 at r1 (raw file):

Previously, gbrodman wrote…

Potentially test deletion / modification of existing values in the premium list?

Done.

Copy link
Member Author

CydeWeys left a comment

PTAL

Reviewable status: 0 of 18 files reviewed, 3 unresolved discussions (waiting on @gbrodman)

Copy link
Collaborator

gbrodman left a comment

:lgtm:

Reviewable status: 0 of 18 files reviewed, all discussions resolved

@CydeWeys CydeWeys merged commit 8f67c4b into google:master Jan 2, 2020
6 of 7 checks passed
6 of 7 checks passed
code-review/reviewable 18 files left
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
LGTM analysis: Java No new or fixed alerts
Details
cla/google All necessary CLAs are signed
kokoro-foss Kokoro build finished
Details
kokoro-internal Kokoro build finished
Details
@CydeWeys CydeWeys deleted the CydeWeys:premium-lists-always-cloud-sql branch Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.