-
Notifications
You must be signed in to change notification settings - Fork 26
ENG-2608 - Add new user and report APIs, and introduce method overloads #136
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
Conversation
# Conflicts: # bin/build-openapi-yaml.rb
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.
self review
@@ -34,7 +34,7 @@ | |||
[#case "KeyAlgorithm"][#return "KeyAlgorithm?"/] | |||
[#default] | |||
[#if type?starts_with("Collection")] | |||
[#return type?replace("Collection", "List")?replace("UUID", "string")/] | |||
[#return type?replace("Collection", "List")?replace("UUID", "string")?replace("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.
In .NET the collection generic is string
not String
. 1st time we've had to do this in a URL param.
# Conflicts: # bin/build-openapi-yaml.rb
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.
self review part 2
bin/build-openapi-yaml.rb
Outdated
paramobj["schema"] = {} | ||
paramobj["schema"]["type"] = "string" | ||
# Cover Java generics here | ||
paramobj["schema"] = if %w[Collection<String> List<String>].include? jsonparamobj['javaType'] |
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.
Set?
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.
added this (although don't think anything uses it yet)
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.
This change is weird. I am going to speak from the perspective of the typescript client, but I assume that there are similar issues in other languages.
In the resulting typescript client the only time that a method parameter is marked "optional" is on the overloaded methods with loginIdTypes
. Our typescript client is fundamentally wrong in that it says everything is required, but at least you can know that and adjust your compiler rules accordingly.
look at this example
/**
* Retrieves the login report between the two instants for a particular user by login Id, using specific loginIdTypes. If you specify an application id, it will only return the
* login counts for that application.
*
* @param {UUID} applicationId (Optional) The application id.
* @param {string} loginId The userId id.
* @param {number} start The start instant as UTC milliseconds since Epoch.
* @param {number} end The end instant as UTC milliseconds since Epoch.
* @param {Array<String>} loginIdTypes (Optional) the identity types that FusionAuth will compare the loginId to. Defaults to [email, username]
* @returns {Promise<ClientResponse<LoginReportResponse>>}
*/
retrieveUserLoginReportByLoginId(applicationId: UUID, loginId: string, start: number, end: number, loginIdTypes?: Array<String>): Promise<ClientResponse<LoginReportResponse>> {
The JSDoc says both applicationId and loginIdTypes are optional, but only one is typed that way.
Also, with a very superficial glance it might appear to some that retrieveUserByLoginIdWithLoginIdTypes
is missing, as we have in defined in some locations but not others and I might be confused if I looked for the same method in one client but it doesn't exist in another.
Happy to discuss further if you like, but this feels pretty odd
Summary
List<?>
Related PRs