Throw exception if no revision can be found for entity with a given ID #1

Closed
wants to merge 5 commits into
from

Projects

None yet

3 participants

Contributor
hygl commented Jun 4, 2012

No description provided.

@olivergierke olivergierke and 1 other commented on an outdated diff Jun 6, 2012
.../repository/support/EnversRevisionRepositoryImpl.java
@@ -113,7 +114,10 @@ public EnversRevisionRepositoryImpl(JpaEntityInformation<T, ?> entityInformation
Class<T> type = entityInformation.getJavaType();
AuditReader reader = AuditReaderFactory.get(entityManager);
List<? extends Number> revisionNumbers = reader.getRevisions(type, id);
-
+ if (revisionNumbers.isEmpty()) {
olivergierke
olivergierke Jun 6, 2012 Owner

What's the intention behind this change? I have quite a few objections here:

  1. What's the problem with returning an empty Revisions object it? It actually nicely expresses the outcome of the invoked method: no revisions found.
  2. If it makes sense, shouldn't the envers-specific exception only be thrown from Envers itself?
  3. We must not throw a Envers specific exception out of our implementation as this would result in the clients need to know about the actual revisioning infrastructure. If we really decide to go the exception way we either need to find a suitable DataAccessException or introduce a new one.
  4. The exception text should contain use case specific assumptions. The suggestion will be wrong most of the times so there's no real point in having it.
hygl
hygl Jun 6, 2012 Contributor

Conserning the objections:

  1. The exception you get with an oracle database is

Caused by: java.sql.SQLSyntaxErrorException: ORA-00936: Ausdruck fehlt

  This definitly does not help to locate the error. 
  1. Ok, then we should define a spring-data-envers specific exeption?
  2. see 2.
  3. I am fine with a different text. Perhalbs something more general.
olivergierke
olivergierke Jun 6, 2012 Owner

Not sure I get you. You don't catch any exception here. Is it maybe thrown from the call to getEntitiesForRevisions(…) call following? If so, I'd vote for a simple return revisionNumbers.isEmpty() ? new Revisions(Collections.emptyList()) : // getEntitiesForRevisions….

hygl
hygl Jun 6, 2012 Contributor

Ok, fine with me.

@olivergierke olivergierke was assigned Jun 6, 2012
@olivergierke olivergierke added a commit that referenced this pull request Jun 6, 2012
@hygl @olivergierke hygl + olivergierke #1 - Fixed example in readme.md. 1c6ee93
@olivergierke olivergierke added a commit that referenced this pull request Jun 6, 2012
@olivergierke olivergierke #1 - Improving test infrastructure.
Added Hamcrest library in 1.2.1. Set up JUnit dependency to 4.10 and
not include dependencies. Set up surefire plugin to use this artifact
for test execution.
ac6d3ea
@olivergierke olivergierke added a commit that referenced this pull request Jun 6, 2012
@hygl @olivergierke hygl + olivergierke #1 - Avoid lookup of entities if no revision numbers found.
We now eagerly return an empty Revisions instance in case we don't
find revision numbers for an entity with a given ID. Not doing so 
caused issues with database vendors as they might expect at least a 
single revision number.
26a51e5
Owner

Slightly polished and squashed the commits and merged them into master.

Is this fix still available in version 0.3.0 because the following code

@RequestMapping("/revisions/{ID}")
@ResponseBody
public List<Revision<Integer, Client>> getRevisions(@PathVariable("ID") String id,Pageable p)
{
    Page<Revision<Integer, Client>> revisions = clientsRepository.findRevisions(id, p);
return revisions.getContent();
}

leads to the following exception when i pass a non-existent Client object ID.

Hibernate: select client_aud0_.REV as col_0_0_ from clients_AUD client_aud0_ cross join AuditRevisionEntity auditrevis1_ where client_aud0_.id=? and client_aud0_.REV=auditrevis1_.id order by client_aud0_.REV asc
Hibernate: select this_.id as id1_0_0_, this_.timestamp as timestam2_0_0_, this_.auditor as auditor3_0_0_ from AuditRevisionEntity this_ where this_.id in ()
23:27:06.893 [tomcat-http--32] ERROR org.hibernate.engine.jdbc.spi.SqlExceptionHelper - You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ')' at line 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment