Skip to content

Do not use easily-misread glyphs in Firestore auto-IDs. - #4590

Closed
dhermes wants to merge 2 commits into
masterfrom
autoid-no-confusing-chars
Closed

Do not use easily-misread glyphs in Firestore auto-IDs.#4590
dhermes wants to merge 2 commits into
masterfrom
autoid-no-confusing-chars

Conversation

@dhermes

@dhermesdhermes commented Dec 13, 2017

Copy link
Copy Markdown
Contributor

This PR removes {'i', 'I', 'l', 'o', 'O', '0', '1'} from the set of characters used in automatically-generated IDs, because it is easy to mis-read them if you are in a situation where you need to do that.

Exact set of blacklisted characters is loaned from Django's make_random_password.

/cc @schmidt-sebastian

Was #4107

Luke Sneeringerand others added 2 commits December 13, 2017 15:23
Luke Sneeringer@dhermes
Do not use easily-misread glyphs in auto-IDs.
d3884d8
@dhermes
Updating from 20 to 21 auto-id chars.
86ce9a4
This way the entropy is preserved after dropping the alphabet
from 62 to 55 characters:
>>> (62. / 55.)**20
10.979435205204474
>>> 55**20 < 62**20 < 55**21
True
@dhermesdhermes added api: firestore Issues related to the Firestore API. type: cleanup An internal cleanup or hygiene concern. labels Dec 13, 2017
@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 13, 2017
@dhermes

dhermes commented Dec 14, 2017

Copy link
Copy Markdown
ContributorAuthor

@lukesneeringer This is the 3rd time you or I have sent this PR (once was in a private repo) 😀

The previous two were a no-go because we were waiting for a thumbs up from the Firestore team.

@chemelnucfin

chemelnucfin commented Dec 14, 2017

Copy link
Copy Markdown
Contributor

@lukesneeringer@dhermes My fault. I just thought it has been approved and was just sitting, so I merged it.

@chemelnucfinchemelnucfin added do not close do not merge Indicates a pull request not ready for merge, due to either quality or timing. status: awaiting information and removed do not close do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Dec 14, 2017
@lukesneeringer

lukesneeringer commented Dec 19, 2017

Copy link
Copy Markdown
Contributor

@dhermes This is a pretty minor change. Either merge it or close it and let's move on with our lives. :-)

schmidt-sebastian
schmidt-sebastian approved these changes Dec 21, 2017
View reviewed changes
Comment threadfirestore/tests/unit/test_collection.py
self.assertEqual(result, mock_result)

mock_calls = [mock.call(_AUTO_ID_CHARS)] * 20
mock_calls = [mock.call(_AUTO_ID_CHARS)] * 21

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermesdhermes closed this Dec 21, 2017
@dhermes
dhermes deleted the autoid-no-confusing-chars branch December 21, 2017 19:03
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@schmidt-sebastianschmidt-sebastianschmidt-sebastian approved these changes
@lukesneeringerlukesneeringerlukesneeringer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@lukesneeringerlukesneeringer

Labels

api: firestoreIssues related to the Firestore API.cla: yesThis human has signed the Contributor License Agreement.status: awaiting informationtype: cleanupAn internal cleanup or hygiene concern.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dhermes@chemelnucfin@lukesneeringer@schmidt-sebastian@googlebot