-
Notifications
You must be signed in to change notification settings - Fork 16
Make dataset alias comparisons case-insensitive #537
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
Make dataset alias comparisons case-insensitive #537
Conversation
984c4a0 to
dd03903
Compare
0ab958b to
4f0a219
Compare
4f0a219 to
24ff5c4
Compare
zaychenko-sergei
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
I can see that you've touched the casing of accounts as well. On today account names are case-sensitive in all implementations of
AuthenticationProvidertrait. These would need to take into account that "Wasya" and "wasya" are the same accounts. Before you start digging into this direction, plz make a quick check if GitHub accounts are case-sensitive or not. It seems they are case-insensitive, but double check. -
I think what is done in the repositories implementation is not enough. You need to make methods like
resolve_dataset_refto be case-insensitive as well. Note that in multi-tenant repos account names are used in datasets folder names, and in single-tenant local repo the dataset name is used as name of folder. Basically, the entire infrastructure of dataset repositories should be lowercasing all objects and references. This, btw, would fix some potential bugs on Windows platform, where file names are case-insensitive. -
I'd like to see more tests. Repository changes for sure. Might be interesting to ensure our HTTP and GraphQL APIs are properly aligned with casing - in HTTP case you need to be careful with URL matching middlewares.
s373r
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a minor comment added
7c158aa to
bce4a15
Compare
bce4a15 to
c4788ba
Compare
|
Summary on the decisive discussion:
|
52c5e93 to
9594777
Compare
d2160c0 to
37dbbd2
Compare
6162ef5 to
b657167
Compare
sergiimk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things mostly look good, but I have doubts in dataset_repository_local_fs.rs. I suggested one extra test to see if my worries are correct.
Sorry you had to deal with all the ugly complexity of the local fs repo - I really hope it will soon go away after we stop storing datasets in account/name directories.
601690b to
60267d6
Compare
60267d6 to
7a2eb00
Compare


Description
Closes: Make dataset names and aliases comparisons case-insensitive
Checklist before requesting a review