-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
KTOR-5225 Add callbacks to save application state for OAuth2 #3282
Conversation
val extraParameters: Parameters = Parameters.Empty, | ||
) : OAuthAccessTokenResponse() { | ||
|
||
public var state: String? = null |
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.
just a question: is it expected, that while OAuth2
is data class
this new property will not appear in copy
/componentN
/equals
/hashCode
?
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.
It's really difficult to add it without breaking binary compatibility. In practice, I don't expect users to do copy
/componentN
on principals. And for equals
and hashcode
, having accessToken
there should be enough.
@@ -117,6 +118,40 @@ public sealed class OAuthServerSettings(public val name: String, public val vers | |||
emptyList(), | |||
accessTokenInterceptor | |||
) | |||
|
|||
@Deprecated("This constructor will be removed", level = DeprecationLevel.HIDDEN) |
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.
minor: "This constructor will be removed" -> "Binary compatibility with 2.x"?
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.
IMO, deprecation messages should be for users explaining why this is deprecated, not for us explaining why it is kept.
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.
AFAIK, declarations with level=HIDDEN
will be not visible to user, until they open source code. They even should not feel this change at all, as new parameter has default value, but ok.
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.
Yes, but for consistency, I prefer it this way.
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.
lgtm
https://youtrack.jetbrains.com/issue/KTOR-5225