Fix CreateRequest chaining and provider ID extraction.#399
Fix CreateRequest chaining and provider ID extraction.#399micahstairs merged 5 commits intoidp-configfrom
Conversation
hiranya911
left a comment
hiranya911
left a comment
There was a problem hiding this comment.
Looks pretty solid. A few ideas for improvement.
| * in this class. | ||
| */ | ||
| public abstract static class CreateRequest { | ||
| public abstract static class CreateRequest<T extends CreateRequest<T>> { |
There was a problem hiding this comment.
Lets call this AbstractCreateRequest or ProviderConfigCreateRequest and reduce overusing the name CreateRequest. This also doesn't need to be a nested class. Might be cleaner to put it elsewhere.
There was a problem hiding this comment.
I like the idea of calling it AbstractCreateRequest instead of CreateRequest. However, I don't think we should move this into a separate class. Keeping it together is more consistent with all of our other CreateRequest/UpdateRequest classes.
On a somewhat related note, should AuthProviderConfig be renamed to AbstractProviderConfig? (Of course, the decision here might be influenced by our generics decision in #400.)
There was a problem hiding this comment.
I prefer the shorter name ProviderConfig for this type (it's already nested in the auth package so the "Auth" part is kind of redundant). Also may be this doesn't need to be an abstract class at all? Is there any behavior that child classes are expected to implement, or are they just adding more attributes to the parent type?
Perhaps we can just make it a regular class with a package-protected constructor?
There was a problem hiding this comment.
I think renaming to ProviderConfig makes sense given that "Auth" is redundant.
However, I think we should keep it abstract for 2 reasons:
- We never want to instantiate it on its own since it can't be used for anything.
- If we make it concrete then we will be forced to implement
getThisinAuthProvider.AbstractCreateRequest. The issue with this is that we then lose the ability to force its subclasses to implementgetThis. If subclasses forget to overridegetThisthen it would break how the chaining works.
There was a problem hiding this comment.
Does 2 actually apply here? Why would the outer class be required to implement a method of an abstract nested class?
There was a problem hiding this comment.
Sorry, you're correct. However, I think keeping it abstract makes it more clear that we do not intend on instantiating it on its own.
hiranya911
left a comment
hiranya911
left a comment
There was a problem hiding this comment.
LGTM with a suggestion, and a comment on the naming and abstract-ness. Feel free to address the latter in separate PRs.
This makes the base `CreateRequest` setters return the proper instance type, so that methods can be chained. This also makes it so that the provider ID can be parsed from the resource name. A package private `getProviderId()` method was also added, which will be needed by the `FirebaseUserManager` class when the provider config operations are added there. Also renamed `AuthProviderConfig` to `ProviderConfig` since "Auth" is redundant with the package name. This work is part of adding multi-tenancy support (see issue #332).
3 participants
This makes the base CreateRequest setters return the proper instance type, so that methods can be chained. This also makes it so that the provider ID can be parsed from the resource name.
A package private getProviderId() method was also added, which will be needed by the FirebaseUserManager class when the provider config operations are added there.
This work is part of adding multi-tenancy support (see issue #332).