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

Proper interfaces for token, client and identity objects #57

Closed
Wicpar opened this issue Apr 12, 2019 · 8 comments
Closed

Proper interfaces for token, client and identity objects #57

Wicpar opened this issue Apr 12, 2019 · 8 comments

Comments

@Wicpar
Copy link

Wicpar commented Apr 12, 2019

Is your feature request related to a problem? Please describe.
A lot of database requests can be avoided if we can use our own implementations, i get 10ms overheads every time an operation is performed, and it accumulates rapidly. With proper interfaces caching could be used.

Describe the solution you'd like
Replace the data classes

nl.myndocs.oauth2.client.Client
nl.myndocs.oauth2.identity.Identity 
nl.myndocs.oauth2.token.AccessToken
nl.myndocs.oauth2.token.CodeToken
nl.myndocs.oauth2.token.RefreshToken

with interfaces
the data classes become the default implementation
token converters would need to be supplied to the config to avoid casting errors

Describe alternatives you've considered
shooting myself in the head because it's impossible to tiptoe around the issues as those classes are final.

@adhesivee
Copy link
Collaborator

adhesivee commented Apr 12, 2019

I am not sure how interfacing the data classes would help you preventing from having multiple queries. For example why couldn't caching be solved in TokenStore.
You could solve caching in the (example) implementation:

class DatabaseTokenStore: TokenStore  {
  val cachedAccessTokens = mutableMapOf<String, AccessToken>()

  override fun accessToken(token: String): AccessToken? {
    if (cachedAccessTokens .contains(token)) {
      return cachedAccessTokens[token]
    }

    // return results from databsae if not in cache
}

Wouldn't this solve your case?

@Wicpar
Copy link
Author

Wicpar commented Apr 15, 2019

My issue is the conversion eating up performance for no good reason.
additionally in a REST API a cache like you propose would create a huge mess when the token is created by one instance and deleted by an other. Database caching takes in account the lifetime of the request and doing that manually would be prone to bugs and regressions.

We minimize maintenance, not development cost, and hacks are extremely high in maintenance.

I am going to do a fork when i get around to optimization.

@Wicpar Wicpar closed this as completed Apr 15, 2019
@adhesivee
Copy link
Collaborator

I am confused, first you started if it is because of the amount of queries, but then it seems to be about conversion of objects. Perhaps I could help if you would have a clear example.

@Wicpar
Copy link
Author

Wicpar commented Apr 15, 2019

The object conversion requires exposed to do additional database queries to fetch the DAO, and caching like in your example would create a security vulnerability as a token deletion from another instance would not clear the other instance's caches.

Everything is simpler if you let people have implementation freedom, and interfaces are the way to go if you don't want to rework everything with generics.

I wanted to see if you are interested in changing that, but it will be easier for everybody if i do a fork when i get around to optimizing the server. I will also add coroutine support while i am at it.

@adhesivee
Copy link
Collaborator

To be clear the psuedo code what I provided here was just an example.
I used data classes to have better seperation between storage and data representation to avoid these ever getting mixed. By making the store flexible, you will have a central place to resolve it. The API will be less clear if we interface data classes.

To me it is still unclear what you want. Do you mean you lack some extra properties that you want to pass around with the data classes? If that is the case, I think a metadata field adds more value (like Identity then interfacing it.

@Wicpar
Copy link
Author

Wicpar commented Apr 15, 2019

I used data classes to have better seperation between storage and data representation to avoid these ever getting mixed.

That is what i am complaining about, the Exposed DAO system requires them to be mixed and this puts me in a pickle. This decision should be up to the implementation.

If you did intend this i will gladly make a fork.

@adhesivee
Copy link
Collaborator

So would metadata on other data classes help you? This is something which can be done quite easily

@Wicpar
Copy link
Author

Wicpar commented Apr 16, 2019

No, thanks.

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

No branches or pull requests

2 participants