search

arangodb / spring-data

Spring Data ArangoDB
https://www.arangodb.com/docs/stable/drivers/spring-data-getting-started.html
Apache License 2.0
111 stars 56 forks source link

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

Closed aburmeis closed 1 year ago

aburmeis commented 1 year ago
rashtao commented 1 year ago

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

Can you rebase https://github.com/arangodb/spring-data/pull/280 on next branch?

aburmeis commented 1 year ago

@rashtao rebased on next

aburmeis commented 1 year ago

@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 commented 1 year ago

@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

https://github.com/arangodb/spring-data/pull/280/commits/de34048aa71149535fffb79cc561e950d08e262b#diff-09eede990db221c2dce7da3fcfa9341f026b8ae728f567d10e1dac1e04ce4382L158

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

https://github.com/arangodb/spring-data/pull/280/commits/5992478b2e372acb86b56329cc483358b564b05e#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:

https://github.com/arangodb/spring-data/pull/280/commits/5992478b2e372acb86b56329cc483358b564b05e#diff-f72f2d932f53727c224eeff419c1177473b44db197a12fcb147a1022c7819b77L391-L396

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

aburmeis commented 1 year ago

created new issue #281 for the exception handling