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

[DE-453] #250 Spring Boot 3 #280

Merged
merged 49 commits into from
Sep 1, 2023
Merged

Conversation

aburmeis
Copy link
Contributor

@aburmeis aburmeis commented Aug 2, 2023

  • support Spring Boot 3, use JDK 17
  • drop JodaTime support
  • deprecate fulltext index support
  • remove old deprecated AbstractArangoConfiguration
  • migrate tests to JUnit 5
  • fix limit of document-to resolver
  • improved exception handling on failing resolver
  • some small code improvements using post java 8 language features
  • document contribution

@cla-bot cla-bot bot added the cla-signed label Aug 2, 2023
@aburmeis aburmeis force-pushed the spring-boot-3 branch 5 times, most recently from 2b1a147 to 4419b52 Compare August 2, 2023 15:19
@rashtao
Copy link
Collaborator

rashtao commented Aug 4, 2023

Thanks for your contribution!
I also think that in the next major release we should upgrade both:

  • Spring Framework to version 6
  • Java Driver to version 7

Can you rebase #280 on next branch?

@aburmeis
Copy link
Contributor Author

aburmeis commented Aug 6, 2023

@rashtao rebased on next

@rashtao rashtao changed the title #250 Spring Boot 3 [DE-453] #250 Spring Boot 3 Aug 31, 2023
@rashtao rashtao self-requested a review September 1, 2023 13:39
Copy link
Collaborator

@rashtao rashtao left a comment

Choose a reason for hiding this comment

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

Hi @aburmeis ,
thanks again for your great contribution!

I reverted some commits unrelated to the topic of this PR, for which we should pay more attention before merging. Some could have unforeseen effects, since not fully tested. Feel free to open new PRs for them.

More in detail I reverted:

  • reverted errors handling changes: error translation is not complete and should be based on both http status codes and error nums from db. Eg. HTTP 412 does not always represent an optimistic locking exception, but it could also be returned due to queue time violation.
  • Revert "less ensure index calls": changing or dropping indexes can be an heavy operation on big databases. I would like avoiding it automatically, or at least having a 2 steps confirmation, e.g. an additional property for enabling it.
  • Revert "Readiness for Spring Boot 3? #250 deleteById by contract must not throw: we should check db error num to verify that the exception is about the document not existing and be sure it is not about something else.

@rashtao rashtao merged commit fa6ca92 into arangodb:master Sep 1, 2023
@aburmeis
Copy link
Contributor Author

aburmeis commented Sep 4, 2023

@rashtao thanks for handling the pr!

For the index handling change I am sorry, I should have done this in an extra PR.

The change to deleteById() and query() error handling are bugfixes imho. deleteById() has a contract telling unknown ids are ignored (see spring data javadoc). Using derived or annotated queries you currently have to deal with ArangoDbException instead of DataAccessExceptions as you would expect from a Spring Data implementation. To do such a change, a major update as this will be anyway, would be the best time to fix this.

@rashtao
Copy link
Collaborator

rashtao commented Sep 5, 2023

@aburmeis

I agree, I reverted those changes from the PR just to be able to move forward faster, reducing the scope of it and avoiding to merge things that need further changes. Feel free to open new PRs for them.

Wrt deleteById(), the problem I saw in the code is that it catches InvalidDataAccessResourceUsageException

de34048#diff-09eede990db221c2dce7da3fcfa9341f026b8ae728f567d10e1dac1e04ce4382L158

which is thrown in case of any HTTP response status code 404:

5992478#diff-180be60e11c007d00648f534e501ad45699a7c334dbe766c1a2229a103250e0fR59

This is incorrect, since 404 could also be returned if the db or the collection are not found, in which cases an exception should be thrown.

To restrict the case to document not found, a match on errorNum == 1202 should be added, see https://github.com/arangodb/arangodb/blob/devel/lib/Basics/errors.dat#L96

About other exceptions handling, as I wrote above, the translation does not encompass all the cases and could produce misleading effects (e.g. throwing OptimisticLockingFailureException in case of timeouts). Also here, we can work together in a new PR.

About exception translation in query(), I am sorry, but I overlooked it and erroneously reverted it:

5992478#diff-f72f2d932f53727c224eeff419c1177473b44db197a12fcb147a1022c7819b77L391-L396

Feel free to re-add it in a new PR, or I would do it if you do not mind.

@aburmeis
Copy link
Contributor Author

aburmeis commented Sep 5, 2023

created new issue #281 for the exception handling

@aburmeis aburmeis deleted the spring-boot-3 branch September 12, 2023 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants