-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Adapt tests that expected a DuplicateKeyException #2113
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
Adapt tests that expected a DuplicateKeyException #2113
Conversation
Hi @vpavic. Would you mind sharing your feedback about this PR? Or maybe you have another solution to share. Thank you. |
Hi @marcusdacoregio - I'll try to take a closer look at this over the next day or two. Hopefully that's OK, as I see no due date set on the 3.0.0-M3 milestone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After taking a look at this, my opinion is that due to the JDBC SessionRepository
having some quite specific error handling scenarios that have been established over the years (based on user feedback, mostly involving high concurrency use cases), there is a need for a more sophisticated SQL exception translation than the one that's present by default in Spring Framework starting with 6.0.
So basically, what I'm saying is that the JdbcTemplate
that's configured in Spring Session's JDBC configuration infrastructure should set up an exception translator that's capable of telling DuplicateKeyException
apart from DataIntegrityViolationException
.
Note that not ensuring such behavior out of the box effectively means that catch
blocks expecting DuplicateKeyException
are dead code, unless users configure JdbcIndexedSessionRepository
by themselves supplying JdbcTemplate
with appropriate exception handling configured.
Also, I don't think this warrants being labeled as breaking change (label type: breaks-passivity
) since there's really nothing breaking in the Spring Session change.
if (logger.isTraceEnabled()) { | ||
logger.trace("Not able to create session attributes", ex); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call to log this - I'd even consider logging it on debug level.
Minor nit: I'd move logging code beneath the comment block so that the intent of the whole catch
block is described first (more readable IMO).
if (logger.isTraceEnabled()) { | ||
logger.trace("Not able to create session attributes", ex); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same things to consider as in the other comment.
Thank you @eleftherias and @vpavic. With your valuable feedback, I've decided to use the |
Closes gh-2108