Skip to content
This repository has been archived by the owner on Dec 9, 2020. It is now read-only.

Change ConsumerId and ResourceOwnerId on TokenData to string #11

Open
mcintyre321 opened this issue Jan 3, 2013 · 2 comments
Open

Change ConsumerId and ResourceOwnerId on TokenData to string #11

mcintyre321 opened this issue Jan 3, 2013 · 2 comments

Comments

@mcintyre321
Copy link

Would it be acceptable to change ConsumerId and ResourceOwnerId to strings as I don't use numeric Ids in my app?

I was thinking that this could be implemented by making the classes use a generic parameter (e.g. IOUthIssue , and perhaps creating some abstract base implementation with overloads for new-ing up TTokenData objects.

I can do this, and send you a pull request, but before I do, is there any reason why this is misguided?

many thanks,

Harry

@micahlmartin
Copy link
Owner

I think changing this will require quite extensive rework. It's definitely doable. Adding one more layer of abstraction or extensibility gives me a little concern as I feel like users already have a slew of interfaces they have to implement. I say give it a shot and send me a pull request. The idea behind numbers was based on the assumption that the majority of people would be using a long in their database. How is it that your are storing your data using strings? Is it a guid or something?

@mcintyre321
Copy link
Author

Yes, I use guids, and I think a lot of people who use NoSql storage will also use string based keys.

If I add string keys, I need to come up with a different algorithm for serializing the TokenData than the one you are using, as it interleaves the byte[8] objects. I was thinking I could just use binary serialization (https://gist.github.com/4451344) to serialize the token data - is there a reason you interleave the bytes using for loops? This would obviously break tokens if this were to be used with the int-based token data.

The code changes are going to be larger, and the codebase more complex, if I maintain backward comparability with your existing TokenData and serialization format rather than just switching the class out. Is that level of compatibility required?

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

No branches or pull requests

2 participants