Support text/plain content type in handleAccessTokenResponse #20
Conversation
This reverts commit f993e07.
# Conflicts: # lib/src/authorization_code_grant.dart
|
I'm also going to modify this to support the |
|
It looks like this converts all the line endings to Windows newlines, which causes a giant diff. Can you undo that? |
|
Will do!!! |
|
Fixed! |
lib/src/credentials.dart
Outdated
| @@ -57,6 +57,10 @@ class Credentials { | |||
| /// expiration date. | |||
| final DateTime expiration; | |||
|
|
|||
| /// A function used to parse parameters out of responses from hosts that do not | |||
| /// respond with application/json or application/x-www-form-urlencoded bodies. | |||
| final Function getParameters; | |||
There was a problem hiding this comment.
This should also have the full function type, and it should be private.
| parameters = DelegatingMap.typed(untypedParameters); | ||
| } on FormatException { | ||
| validate(false, 'invalid JSON'); | ||
| parameters = getParameters(contentTypeString, response.body); |
There was a problem hiding this comment.
It seems wrong that we're passing the full content type here, but only the mime type in _handleErrorResponse() below. Why not just pass in the parsed MediaType in both cases?
Also, if whatever we pass in can be null, that should be clearly documented.
lib/src/parameters.dart
Outdated
| import 'package:collection/collection.dart'; | ||
| import 'package:http_parser/http_parser.dart'; | ||
|
|
||
| typedef Map<String, dynamic> GetParameters(String contentType, String body); |
| if (condition) return; | ||
| throw new FormatException('Invalid OAuth response for "$tokenEndpoint": ' | ||
| '$message.\n\n${response.body}'); | ||
| } |
There was a problem hiding this comment.
This still needs to exist; right now, validation outside of getParameters() won't include the token endpoint or the response body.
lib/src/parameters.dart
Outdated
|
|
||
| typedef Map<String, dynamic> GetParameters(String contentType, String body); | ||
|
|
||
| void validate(bool condition, String message) { |
There was a problem hiding this comment.
I don't think it's worth defining this versus just manually writing if statements in parseJsonParameters().
…ent-type strings, we are passing MediaTypes. Thus, if a content-type string is null, an ArgumentError will be thrown, instead of FormatException.
|
Sorry this took me so long to get through, but finally made the requested changes. I've changed one of the tests from expecting a FormatException to an ArgumentError; this is just because now instead of passing around content-type strings, we're passing MediaTypes. |
|
Please merge in the latest |
|
Finally merged the master inwards |
|
Looks like you accidentally committed your |
|
Removed it now, sorry. |
|
It looks like your change is causing a browser test failure. Can you investigate? |
|
So, it looks like though an I'm wondering if it would be acceptable to change the check for |
|
The easiest thing to do would probably be to just keep explicitly throwing a |
|
Sounds good to me! Hopefully the build succeeds this time. |
[skip ci]
|
Looks good! Thanks for all your work on this. |
|
Always happy to contribute! |
This should resolve dart-lang/tools#303.