-
Notifications
You must be signed in to change notification settings - Fork 35
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
QueryParam Backcompat #1587
QueryParam Backcompat #1587
Conversation
6a4e848
to
cb20b48
Compare
cb20b48
to
24881eb
Compare
common/src/main/java/com/microsoft/identity/common/internal/util/QueryParamsAdapter.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/microsoft/identity/common/internal/util/QueryParamsAdapter.java
Outdated
Show resolved
Hide resolved
common/src/test/java/com/microsoft/identity/common/internal/util/QueryParamsAdapterTest.java
Outdated
Show resolved
Hide resolved
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 looks like we're trying to write the most efficient form, which should abstract away the type. Before we do this, we should understand why that's not happening.
*/ | ||
public class QueryParamsAdapter extends TypeAdapter<List<Map.Entry<String, String>>> { | ||
public class QueryParamsAdapter extends TypeAdapter<List<Pair<String, String>>> { |
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.
How does this now work with new MSAL? I suppose that's going to send Map.Entry?
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.
the new MSAL (not in prod) sends Map.Entry, the old one sends Pair.
Please also read the finding i just added below - this TypeAdapter is never properly used.
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.
We should document this in the release notes for MSAL/ADAL.. As part of how to upgrade to new major version.
right?
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.
I think with this change now even new one sends Pair..so I don't expect there to be any changes needed for upgrade. @rpdome can confirm.
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. no change.
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.
the new one will serialize Map.Entry into the very same format as Pair.
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.
I had to make this change in Authenticator code, with the first RC build, that's why I thought..
Maybe I will need to revert that with the new RC build :)
@AdamBJohnsonx Unfortunately, we shipped a bug from the beginning - we're supposed to specify the TypeToken of This means we're stuck with it, unless we want to make a broker-msal protocol breaking change. If we migrate to the right format To prevent the confusion, I'll just kill the adapter. |
Alternatively, i could make the adapter of List<Map.Entry> to return List's format. |
What does GSON default adapter do? I suppose it is handling Pair correctly? Can the default not handle Map.Entry? |
common/src/test/java/com/microsoft/identity/common/internal/util/QueryParamsAdapterTest.java
Outdated
Show resolved
Hide resolved
you get something like this pair: [{"first":"eqp1","second":"1"},{"first":"eqp2","second":"2"}] The default can handle map, but we're stuck with the pair format (since there will always be older MSAL sending it, or an older Broker that expects it) |
common/src/main/java/com/microsoft/identity/common/internal/util/QueryParamsAdapter.java
Outdated
Show resolved
Hide resolved
Bleah. So the thing you *can * do is to code the read side of this adapter so that it can understand any of these formats, rev the broker protocol version for the compact format, and choose how you want to send it. We should definitely break any tie between the structure of code being used to hold the data and how that data is transmitted and understood, however it happens. |
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.
Looks good to me.
Older MSAL/Common serializes broker request request from Pair.
Even though we're ditching Pair already (for common4j - Pair is an android class), we need to be still able to support it.