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

Spaces in model ids not supported. #69

Closed
thomasfedb opened this issue May 5, 2015 · 5 comments
Closed

Spaces in model ids not supported. #69

thomasfedb opened this issue May 5, 2015 · 5 comments
Labels

Comments

@thomasfedb
Copy link
Contributor

I have an external database which has text identifiers, some of which end in a trailing space.

When using global ids with these records, the global ids generated fail to be parsed, and therefore lookup also fails.

Perhaps model ids should be urlencoded?

@thomasfedb
Copy link
Contributor Author

Here's an example of this issue in action:

irb(main):015:0> Tutorial.find("R-SKC ").id
=> "R-SKC "
irb(main):019:0> Tutorial.find("R-SKC ").to_global_id.to_s
=> "gid://tenjin/Tutorial/R-SKC "
irb(main):020:0> GlobalID.parse("gid://tenjin/Tutorial/R-SKC ")
=> nil

@kaspth
Copy link
Contributor

kaspth commented May 5, 2015

I'm not sure how common this is. Does overriding to_global_id in your model work? You'd probably need a custom locator to whip the model id back into shape.

@matthewd
Copy link
Member

matthewd commented May 5, 2015

@kaspth needing to support trailing spaces might be an edge case, but that's what encoding schemes are for.

AIUI, we're currently insisting (well, assuming) that all identifiers contain only characters that are safe unescaped in a URL path segment. Are we deriving some benefit from that restriction, beyond not having to call a method? 😕

@thomasfedb
Copy link
Contributor Author

@kaspth I think this is less a case of 'trailing spaces are weird, why would anybody need that' and more a case of the fact that ActiveRecord supports a far wider range of values as IDs than GlobalID. It would be nice to fix that.

Also, for the majority of cases, urlencoding the ID should be backwards compatible.

@kaspth
Copy link
Contributor

kaspth commented May 30, 2015

@matthewd @thomasfedb Sorry for being slow here. You're both right, and Active Record supporting many more types of ids is why we should fix this.

@thomasfedb would you be interested in opening a PR? 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants