Switched from oracle.jdbc.datasource.OracleCommonDataSource to oracle.jdbc.datasource.OracleDataSource to fix invalid cast of oracle.jdbc.datasource.OracleCommonDataSource.class to Class<? extends DataSource>.
Added some readability improvements.
Comment From: wilkinsona
Thanks for the pull request.
I'm afraid I'm finding it rather difficult to identify the fix for the invalid class cast exception from the readability improvements. Also, personally speaking, I find a lot of the readability changes harder to read than the original code. For example, the one of streams and Optional while shorter is harder for me to read. Throughout the codebase we avoid using Optional in this way for this sort of reason.
Could you please update the proposal to focus on the fix for the invalid cast and illustrate the problem with a test that fails without the proposal?
Comment From: snicoll
Added some readability improvements.
I second @wilkinsona here. We very much prefer that an issue report (and code change) focuses on the task at hand. Good catch on the fact that the current interface does not implement DataSource, I totally missed that.
I personnel find the readability improvements harder to read that the current arrangement. If you feel differently, we'll have to discuss that in a separate issue.
Comment From: snicoll
@fabio-grassi-gbs I've seen the update but I am not sure that's actually addressing the concern. I can take over if you prefer as this is a regression we should fix before the next milestone.
Comment From: fabio-grassi-gbs
Ok @snicoll , please take over if time is short. Let me know if it is not clear what I meant to improve/fix, which by the way is a minor problem, type safety is only at risk if the source code is modified, not during regular use.
Comment From: snicoll
@fabio-grassi-gbs alright, I'll take over. I think the most important here is to focus on one task at a time. Yes we pass along a type doesn't implement DataSource for Oracle so the type safety issue exists but that doesn't mean we should change DataSourceBuilder the way you did. Thanks for the PR, in any case.
Comment From: fabio-grassi-gbs
Ok, I agree it's a minor issue and mostly cosmetic as method at risk is private. Furthermore I saw ef2fee2 now uses a real DataSource. Thanks.
Comment From: fabio-grassi-gbs
Just one final question if you will and for mere curiosity. I'm not sure I understood why you decided to introduce the subclass OracleCommonDataSourceSettings. The only reason I see is to override getType and than potentially preventing the attempt to instantiate an interface. Is there some deeper reason? TIA.
Comment From: snicoll
The builder can be used so that a suitable connection pool is detected from the classpath or the user specifies the type that they want to use. If someone sets the type to OracleDataSource and wants Spring Boot to configure the url, username and password, an alias is required as described in the constructor. Without this class we won't be able to call the setUser method.
That makes me think that this class should probably be renamed to OracleDataSourceSettings so thank you for that. Having said that, please consider using Gitter going forward if you have design questions like this.