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

Clean up token generation #205

Merged
merged 7 commits into from Aug 12, 2019

Conversation

@gbrodman
Copy link
Collaborator

commented Jul 31, 2019

  • Allow tokenLength of 0
  • If specifying a token length of 0, throw an error if numTokens > 1

This change is Reviewable

Clean up token generation
- Allow tokenLength of 0
- If specifying a token length of 0, throw an error if numTokens > 1

@googlebot googlebot added the cla: yes label Jul 31, 2019

@gbrodman gbrodman requested a review from CydeWeys Jul 31, 2019

@gbrodman gbrodman force-pushed the gbrodman:cleanupGenerateTokens branch from 166951d to 92e6377 Jul 31, 2019

@CydeWeys
Copy link
Member

left a comment

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

a discussion (no related file):
I'm not sure that removing the >0 restriction on the string generators is the way to go, as calling them with 0 is likely to be an error (e.g. use of an unitialized/default-valued int).

Better to address the issue at the callsites, i.e. not call the string generators at all when they're not needed, because a token with a specific value is being created.



core/src/main/java/google/registry/tools/GenerateAllocationTokensCommand.java, line 167 at r1 (raw file):

    checkArgument(
        tokenLength > 0 || numTokens == 1,
        "When specifying a token length of 0, one can only generate one token");

What is the overall intent of this change? That one should be able to create a token with a specific string and no random components whatsoever? If so, it might be best to just add a new parameter to allow passing in the exact token string(s) themselves, rather than shoehorning it into --prefix 'string' --length 0. And then add a new restriction that you can either pass in the token strings XOR the prefix and lengths.

@gbrodman
Copy link
Collaborator Author

left a comment

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

a discussion (no related file):

Previously, CydeWeys (Ben McIlwain) wrote…

I'm not sure that removing the >0 restriction on the string generators is the way to go, as calling them with 0 is likely to be an error (e.g. use of an unitialized/default-valued int).

Better to address the issue at the callsites, i.e. not call the string generators at all when they're not needed, because a token with a specific value is being created.

I might disagree, in that if you ask me to generate a random string with length 0, wouldn't the technically correct response to be to return the empty string?



core/src/main/java/google/registry/tools/GenerateAllocationTokensCommand.java, line 167 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

What is the overall intent of this change? That one should be able to create a token with a specific string and no random components whatsoever? If so, it might be best to just add a new parameter to allow passing in the exact token string(s) themselves, rather than shoehorning it into --prefix 'string' --length 0. And then add a new restriction that you can either pass in the token strings XOR the prefix and lengths.

That is correct. That strategy (passing in a specific string or set of strings) would be different from what we talked about before but would mirror UpdateOrDeleteAllocationTokensCommand nicely.

@CydeWeys
Copy link
Member

left a comment

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

a discussion (no related file):

Previously, gbrodman wrote…

I might disagree, in that if you ask me to generate a random string with length 0, wouldn't the technically correct response to be to return the empty string?

The most likely thing is that you made an error in asking for a random string with length 0. It's correct to treat this as an error condition. Being technically correct, but not respecting intents, is how you get bugs. The intent of a random string generator is never to return an empty string, which is by definition not random and contains 0 bits of entropy. Great way to get bugs if you allow that.


@gbrodman
Copy link
Collaborator Author

left a comment

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

a discussion (no related file):

Previously, CydeWeys (Ben McIlwain) wrote…

The most likely thing is that you made an error in asking for a random string with length 0. It's correct to treat this as an error condition. Being technically correct, but not respecting intents, is how you get bugs. The intent of a random string generator is never to return an empty string, which is by definition not random and contains 0 bits of entropy. Great way to get bugs if you allow that.

Fair enough. You're right in that with simple functions like this you can definitely know the intent.


@CydeWeys
Copy link
Member

left a comment

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


core/src/main/java/google/registry/tools/GenerateAllocationTokensCommand.java, line 167 at r1 (raw file):

Previously, gbrodman wrote…

That is correct. That strategy (passing in a specific string or set of strings) would be different from what we talked about before but would mirror UpdateOrDeleteAllocationTokensCommand nicely.

So if I pass in, say, 100 token strings, which is more than the batch size ... does this work?


core/src/main/java/google/registry/tools/GenerateAllocationTokensCommand.java, line 159 at r2 (raw file):

    checkArgument(
        tokenLength > 0,
        "Token length should not be 0. To generate exact tokens, use the --tokens parameter.");

It might be worth moving the command validation logic into a separate method than the actual execution logic at this point.


core/src/main/java/google/registry/tools/GenerateAllocationTokensCommand.java, line 229 at r2 (raw file):

  private Stream<String> getNextTokenBatch(int tokensSaved) {
    if (tokenStrings != null) {
      return tokenStrings.stream();

Is this a batch or is this just all of them?

@gbrodman
Copy link
Collaborator Author

left a comment

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


core/src/main/java/google/registry/tools/GenerateAllocationTokensCommand.java, line 167 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

So if I pass in, say, 100 token strings, which is more than the batch size ... does this work?

Now it does :)

I wasn't familiar with the batch limits before.


core/src/main/java/google/registry/tools/GenerateAllocationTokensCommand.java, line 159 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

It might be worth moving the command validation logic into a separate method than the actual execution logic at this point.

Done.


core/src/main/java/google/registry/tools/GenerateAllocationTokensCommand.java, line 229 at r2 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Is this a batch or is this just all of them?

It'll now batch them

@CydeWeys
Copy link
Member

left a comment

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @gbrodman)


core/src/main/java/google/registry/tools/GenerateAllocationTokensCommand.java, line 167 at r1 (raw file):

Previously, gbrodman wrote…

Now it does :)

I wasn't familiar with the batch limits before.

Could use a test. Don't go crazy; something a little about 21 would work fine. Something like: --tokens a,b,c,d,e,f,g,h,j,j,k,l,m,n,o,p,q,r,s,t,u,v

@CydeWeys
Copy link
Member

left a comment

LGTM

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @gbrodman)

@gbrodman
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @CydeWeys)


core/src/main/java/google/registry/tools/GenerateAllocationTokensCommand.java, line 167 at r1 (raw file):

Previously, CydeWeys (Ben McIlwain) wrote…

Could use a test. Don't go crazy; something a little about 21 would work fine. Something like: --tokens a,b,c,d,e,f,g,h,j,j,k,l,m,n,o,p,q,r,s,t,u,v

I believe I already added this, with testSuccess_largeNumberOfTokens

@gbrodman
Copy link
Collaborator Author

left a comment

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @CydeWeys)


core/src/main/java/google/registry/tools/GenerateAllocationTokensCommand.java, line 167 at r1 (raw file):

Previously, gbrodman wrote…

I believe I already added this, with testSuccess_largeNumberOfTokens

Oops, I mean testSuccess_specifyManyTokens

@gbrodman gbrodman merged commit 89a44f1 into google:master Aug 12, 2019

4 of 7 checks passed

code-review/reviewable 2 files, 1 discussion left (CydeWeys)
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
continuous-integration/travis-ci/pr The Travis CI build passed
Details
kokoro Kokoro build finished
Details

@gbrodman gbrodman deleted the gbrodman:cleanupGenerateTokens branch Aug 12, 2019

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.