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

Support for short GUIDs #3

Open
danieljsummers opened this issue Feb 23, 2019 · 3 comments
Open

Support for short GUIDs #3

danieljsummers opened this issue Feb 23, 2019 · 3 comments
Labels
enhancement New feature or request

Comments

@danieljsummers
Copy link

I attempted to use a short GUID with the token router, and it didn't like it. I checked, and sure enough, this one doesn't support it - yet. Would you entertain a PR for it?

My thought was to check in the GUID parsing logic to see if the captured length was 22 characters, and if so, run it through ShortGuid.toGuid. It may not be the most efficient way to do it (I imagine there's a reason it doesn't use Guid.TryParse on the chunk of captured data), but it would provide support for the format.

It might not be too tough to parse through it in chunks of 4 characters, and grab the bits from there via a lookup table. The lookup table part makes me wonder how efficiently it could be done. I'm definitely not too strong with bit-shifting.

Anyway, let me know. I don't know that I could turn it really quick, but I think it wouldn't take too long.

@gerardtoconnor
Copy link
Member

Hi Daniel, I am not overly familiar with the short Guid, is it just a base64 string representation that is parsed to a 128bit GUID?
If that is the case, you would probably need to tweak the parser to try figure out if its base16 or base64 (22 length).
The reason it does not use Guid.TryParse is that it is slow compared to our specialised implementation, I can't remember exactly but was 4x/5x faster with custom parse logic. The same logic should be useable for base64 (each char fills 4 bytes) to fill out the GUID underlying array using bitshifting etc.

The most tedious part will be making decision tree of wheather base16 or base64 string so that is it still virtually same performance... I would prefer to use a spare format char (%O being use for GUID now) so that does not have to be a runtime choice, but rather router specialised for each.

Do you want to have a bash at a PR or is it bit tough to reason in which case i can try put something in soon?

@gerardtoconnor gerardtoconnor added the enhancement New feature or request label Feb 25, 2019
@danieljsummers
Copy link
Author

Giraffe's default router uses %O for both, so if the feature parity is there, the solution would probably need to use the same format character. Its implementation looks at the length of the input; if it's 22 characters, it assumes ShortGuid. If we can see the length of the token we're parsing, that should be a simple branch statement. (If my assumption is wrong there, please correct me. :) )

Your understanding is correct; it's the GUID as a base64 string. I did some digging over the weekend, and the algorithm I was seeing was that 4 6-bit characters become 3 bytes in the output. I'm sure calling Convert.FromBase64String would have terrible performance, as compared with simply moving the bits around.

I would be up for attempting a PR, especially since the output could be easily tested by calling the Giraffe methods to ensure they were equivalent. I'll fork and work on it, probably starting this evening (US Central).

@gerardtoconnor
Copy link
Member

Cool, maybe for now, don't worry too much on the perf, base 64 is quite messy trying to map to the 16 bytes needed for the guid, 64 char will map to 6bits, 4 chars making 3 bytes, but they usually map in twos / fours in the layout so there may be a lot of partial shift logic that would make it a night mare... if you think you can do it by all means but I realise that it's way more involved then 32 * hex chars. and probably not worth it if you don't have a known perf problem anyway.

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

No branches or pull requests

2 participants