Make OAuth2AuthorizationRequest extendable

[aranoverse]

OAuth2AuthorizationRequest should not be final class the issue link

[pivotal-cla]

@aranoverse Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[jzheaux]

Thanks, @aranoverse. Can you explain more about what you are wanting to do in your application? I ask because it's worth leaving things final until opening them is needed. It's also worth considering whether the Builder class should also be extendible.

[aranoverse]

Thanks, @aranoverse. Can you explain more about what you are wanting to do in your application? I ask because it's worth leaving things final until opening them is needed. It's also worth considering whether the Builder class should also be extendible.

Thank you for noticing this issue. When using the AuthorizationRequestRepository, I want to store some additional data fields. I found that the declaration of AuthorizationRequestRepository has T extends OAuth2AuthorizationRequest, but I discovered that OAuth2AuthorizationRequest is not extendable.

Although I can work around this problem using attributes, from a design perspective, I don't think T extends OAuth2AuthorizationRequest which is a not extendable class is correct.

[sjohnr]

Hi @aranoverse, thanks for your explanation!

Although I can work around this problem using attributes, from a design perspective, I don't think T extends OAuth2AuthorizationRequest which is a not extendable class is correct.

You are correct that this does not seem logically consistent. I have spoken with @jgrandja to confirm that in fact the original idea was to leave the interface flexible in case subclasses of OAuth2AuthorizationRequest were required. Because of attributes and additionalParameters, subclasses have not been needed since this class was introduced in 5.0. Therefore, there are no plans to introduce a subclass and it does not seem necessary to make the class non-final.

When using the AuthorizationRequestRepository, I want to store some additional data fields.

The field additionalParameters is ideal for this situation. I believe you can use that instead of attributes. For now, I'm going to close this PR as it does not seem necessary to open up the class for extension. I will also close the corresponding issue as well.

However, you are correct that the interface AuthorizationRequestRepository does not require generics. We can open a separate issue to address this in 7.0.

If you feel I have misunderstood anything, please let me know.

[aranoverse]

Hi @aranoverse, thanks for your explanation!

Although I can work around this problem using attributes, from a design perspective, I don't think T extends OAuth2AuthorizationRequest which is a not extendable class is correct.

You are correct that this does not seem logically consistent. I have spoken with @jgrandja to confirm that in fact the original idea was to leave the interface flexible in case subclasses of OAuth2AuthorizationRequest were required. Because of attributes and additionalParameters, subclasses have not been needed since this class was introduced in 5.0. Therefore, there are no plans to introduce a subclass and it does not seem necessary to make the class non-final.

When using the AuthorizationRequestRepository, I want to store some additional data fields.

The field additionalParameters is ideal for this situation. I believe you can use that instead of attributes. For now, I'm going to close this PR as it does not seem necessary to open up the class for extension. I will also close the corresponding issue as well.

However, you are correct that the interface AuthorizationRequestRepository does not require generics. We can open a separate issue to address this in 7.0.

If you feel I have misunderstood anything, please let me know.

Thanks 🍻 !