-
Notifications
You must be signed in to change notification settings - Fork 14
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
Make dataset alias comparisons case-insensitive #537
Conversation
984c4a0
to
dd03903
Compare
0ab958b
to
4f0a219
Compare
4f0a219
to
24ff5c4
Compare
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
AuthenticationProvider
trait. 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_ref
to 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.
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
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