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

Check zone id #2617

Merged
merged 3 commits into from
Dec 6, 2023
Merged

Check zone id #2617

merged 3 commits into from
Dec 6, 2023

Conversation

strehle
Copy link
Member

@strehle strehle commented Nov 23, 2023

After discussions in #2606

Add null checks to improve robustness in this dao

add more tests to cover all edge cases

https://sonarcloud.io/summary/new_code?id=cloudfoundry-identity-parent&pullRequest=2617

@cf-gitbot
Copy link

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

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

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

add more tests to cover all edge cases
@strehle strehle requested a review from a team November 23, 2023 16:59
@strehle strehle added this to the 76.27.0 milestone Nov 23, 2023
Copy link
Contributor

@klaus-sap klaus-sap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me

@strehle
Copy link
Member Author

strehle commented Dec 1, 2023

@swalchemist This is a small refactoring in preparation of a pr from @klaus-sap .

Return now an exception before asking JDBC (or DB) about null for a zone.
EmptyResultDataAccessException("Zone id cannot be null", 1);

@tack-sap
Copy link
Contributor

tack-sap commented Dec 4, 2023

@strehle currently the methods do not throw EmptyResultDataAccessException, but only ZoneDoesNotExistException. Would this maybe be a better Exception to throw, as this is something the calling classes may already expect?

@strehle
Copy link
Member Author

strehle commented Dec 4, 2023

@strehle currently the methods do not throw EmptyResultDataAccessException, but only ZoneDoesNotExistException. Would this maybe be a better Exception to throw, as this is something the calling classes may already expect?

ok

Copy link
Contributor

@tack-sap tack-sap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@strehle strehle merged commit ab838d2 into develop Dec 6, 2023
20 checks passed
@strehle strehle deleted the zoneConfigCoverage branch December 6, 2023 13:42
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

4 participants