Update fails to detected when record not found for update

[simplemes-old]

Sorry, but I don't have an example project for this issue. Let me know if you need one.

In the DefaultJdbcRepositoryOperations.update() method, it does not check the result of the ps.executeUpdate() to verify that the record was really updated.

If another process deletes the record, then the update will silently fail. That would be a big problem for my type of application.

Could you throw a 'record not found' exception of some sort? As an alternative, you could have the method return null when the update does not affect any records. I could detect that too.

I assume a simple check for result<>1 would be Ok. I don't know if you will need to make this configurable. I don't need it to be configurable.

Thanks.

[graemerocher]

You can return Number and check the result yourself no?

[simplemes-old]

(Thanks and sorry for the long reply, the key part is the Suggested Change below).

I tried something like this in the repository:

Number update(@Id UUID id, BigDecimal qty)

This works, but I really need to change many fields in the record in memory and then UPDATE the record in the DB. I also tried this approach:

Number update(@Valid @NotNull Order order)

This causes the 'No possible implementations found.' message during compile. I understand the reasoning for that message.

I am still worried about the data corruption issues if the core update() method silently fails. I would have to find some way to disable the update() method. I am also worried about developers who miss this during development. This would be a tough issue to diagnose on a production system.

Since I am already creating sub-class of the DefaultJdbcRepositoryOperations to implement a listener, I also changed it to update a version field for optimistic locking. This works, but the core update() method does not indicate the update did not take place.

Suggested Change Could you change the core method to return null if the SQL update affected zero records? This would avoid changing the signature of the update method and still allow everyone to detect when the update failed.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[mphouston]

Please re-consider making some sort of change to detect this case. I am certain this will cause data integrity issues that are not caught in production. Last week, I spent a few hours tracking down and update that did nothing. Maybe throw an exception if no records updated? Or support the sytnax:

Number update(Order order)

I don't see a way to make the suggestion (return a number) to work with a real-life domain. Most of my records have 10+ columns to update. It would expand the repository with too many update methods or some really wide argument lists (10+ arguments on the update). Clarity would suffer a lot.

If this issue is not addressed, then I will probably add an extra SELECT to make sure the record exists before I do the update. Performance is important, but I have to choose between performance and data integrity, I choose the data :).

[mphouston]

Thanks :). Let me know if you need any feedback from me (I was the creator under my project name).

[mphouston]

Seems like if anyone uses a repository update() method then they have a big data corruption hole lurking in their code. I can't believe that nobody else has complained about it.

I know this is an old issue, but I really see no way to fix the current DefaultJdbcRepositoryOperations class to detect the number of records updated. I have adjusted the class's behavior for other methods by sub-classing it, but I can't access the PreparedStatement.execute() results. Maybe the persisted parameter could be re-purposed to hold the update result?

Any suggestions?

My next attempt is to sub-class these classes and store the record count in a custom transaction status (uggh):

  AbstractSynchronousTransactionManager.executeWrite()
    TransactionStatus.getConnection()
      Connection.prepareStatement()

I would need to use the sub-classes to provide a low-level wrapper on the prepareStatement() to store the updated row count in the status element.

[mikehearn]

I also just hit this and found it surprising that it's not considered an error. I guess backwards compatibility would be an issue at this point as some apps may expect this, but for the next major version bump it'd be good to consider changing this to allow for some sort of update count assertion which defaults to >0.