Skip to content
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

Refactoring: use namedJdbcTemplate bean instead of internal new object #2864

Merged
merged 3 commits into from
May 7, 2024

Conversation

strehle
Copy link
Member

@strehle strehle commented May 2, 2024

Allow to mock the objects in future

e.g. if jdbcTemplate is a mock then sometimes such pattern new NamedParameterJdbcTemplate(jdbcTemplate) causes a not testable code block like in JdbcScimUserProvisioning

For jdbcTemplate we create one instance and re-use it, therefore if NamedParameterJdbcTemplate is used in several beans, then we should do the same

Allow to mock the objects in future

e.g. if jdbcTemplate is a mock then sometimes such pattern
new NamedParameterJdbcTemplate(jdbcTemplate) causes a not testable code block like in JdbcScimUserProvisioning

For jdbcTemplate we create one instance and re-use it, therefore
if NamedParameterJdbcTemplate is used in several beans, then we should
do the same
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/187536573

The labels on this github issue will be updated when the story is started.

@strehle strehle requested a review from a team May 2, 2024 16:35
@peterhaochen47 peterhaochen47 self-requested a review May 3, 2024 20:24
@peterhaochen47
Copy link
Member

peterhaochen47 commented May 3, 2024

Allow to mock the objects in future
e.g. if jdbcTemplate is a mock then sometimes such pattern new NamedParameterJdbcTemplate(jdbcTemplate) causes a not testable code block like in JdbcScimUserProvisioning

Hi! I don't think I understand this point. Could you elaborate? What code block is not testable under JdbcTemplate and how does NamedParameterJdbcTemplate solve that problem?

It looks like the main advantage of NamedParameterJdbcTemplate is that you could perform param substitution on the SQL string in a stricter manner (but I don't see this feature being used in this PR?).

@strehle
Copy link
Member Author

strehle commented May 4, 2024

Allow to mock the objects in future
e.g. if jdbcTemplate is a mock then sometimes such pattern new NamedParameterJdbcTemplate(jdbcTemplate) causes a not testable code block like in JdbcScimUserProvisioning

Hi! I don't think I understand this point. Could you elaborate? What code block is not testable under JdbcTemplate and how does NamedParameterJdbcTemplate solve that problem?

It looks like the main advantage of NamedParameterJdbcTemplate is that you could perform param substitution on the SQL string in a stricter manner (but I don't see this feature being used in this PR?).

The change is, that classes which use this can now be mocked, as done with #2863

Another reason why I wanted this refactoring is that I was wondering for a while, why we create a datasource and jdbcTemplate in a single place -> data-source.xml and re-use this reference in all needed places but the enhance jdbc class NamedParameterJdbcTemplate is always newly generated with each bean creating, see
https://github.com/search?q=repo%3Acloudfoundry%2Fuaa+%22new+NamedParameterJdbcTemplate%22&type=code

I think we will re-use NamedParameterJdbcTemplate more often therefore good to use it in same way we do with underlying jdbcTemplate/datasource

@Tallicia Tallicia added in progress accepted Accepted the issue labels May 6, 2024
…tor/NamedJdbcTemplate

# Conflicts:
#	server/src/main/java/org/cloudfoundry/identity/uaa/resources/jdbc/AbstractQueryable.java
#	server/src/main/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioning.java
#	server/src/test/java/org/cloudfoundry/identity/uaa/scim/jdbc/JdbcScimUserProvisioningTests.java
@strehle strehle merged commit 6c30c85 into develop May 7, 2024
20 checks passed
@strehle strehle deleted the refactor/NamedJdbcTemplate branch May 7, 2024 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants