Make granting catalog roles idempotent for JDBC persistence#4554
Conversation
flyrain
left a comment
flyrain
left a comment
There was a problem hiding this comment.
Thanks a lot for the fix, @snicholasbarton ! Left a few comments.
| QueryGenerator.generateInsertQuery( | ||
| ModelGrantRecord.ALL_COLUMNS, ModelGrantRecord.TABLE_NAME, values, realmId)); | ||
| } catch (SQLException e) { | ||
| if (datasourceOperations.isConstraintViolation(e)) { |
There was a problem hiding this comment.
Narrow swallow looks right, only SQLSTATE 23505 (unique_violation) is suppressed; FK/NOT NULL/CHECK still propagate via the existing throw. Can we double check cockroachDb and H2 to ensure the state is consistent across them?
| * part of the PK. If additional non-PK attributes are added this might change. | ||
| * | ||
| * @param callCtx call context | ||
| * @param grantRec entity record to write, potentially replacing an existing entity record with |
There was a problem hiding this comment.
The body now says "no-op" but @param grantRec still says "potentially replacing an existing entity record with the same key", can we reconcile to one wording? Also worth noting that a duplicate write still triggers grant-records-version bumps in the calling path.
There was a problem hiding this comment.
Updated the param wording.
For the grant-records-version - that's used for caching, right? So after this change - where dupe writes don't throw - we pay an extra write (to the grant records version table) + later an extra read (after the invalidated cache lookup). I would anticipate that in normal use there probably isn't a huge number of these dupes, so maybe that's an acceptable trade.
If we do care about it we could potentially return a flag indicating whether a write was /actually/ made and only increment the version if so? Would potentially want to do that as a separate change.
| new PolarisGrantRecord( | ||
| SECURABLE_CATALOG_ID, SECURABLE_ID, GRANTEE_CATALOG_ID, GRANTEE_ID, PRIVILEGE_CODE); | ||
|
|
||
| assertThatCode(() -> basePersistence.writeToGrantRecords(callCtx, grant)) |
There was a problem hiding this comment.
Optional: also add a negative case asserting non-23505 SQLExceptions still propagate (e.g. via a stub DatasourceOperations), so the swallow stays narrow if someone widens isConstraintViolation later.
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for your contribution, @snicholasbarton !
| QueryGenerator.generateInsertQuery( | ||
| ModelGrantRecord.ALL_COLUMNS, ModelGrantRecord.TABLE_NAME, values, realmId)); | ||
| } catch (SQLException e) { | ||
| if (datasourceOperations.isConstraintViolation(e)) { |
There was a problem hiding this comment.
Would it be more appropriate to use MERGE INTO SQL instead?
There was a problem hiding this comment.
I'm not opposed in theory; if non-PK fields are added later then we probably do want this. Chose not to here for consistency (didn't see any existing direct upsert query patterns), portability (I think the query might be different per PG/Cockroach/H2), and YAGNI (all the fields are part of the PK, so currently we don't actually have anything to update). Happy to revisit if you think it's worth it
There was a problem hiding this comment.
Fair enough 👍 Thanks for narrowing the method name to "unique constraint" violations. Given that we currently have only the PK constraint on this table this change looks pretty solid to me.
| return new ReserveResult(ReserveResultType.OWNED, Optional.empty()); | ||
| } catch (SQLException e) { | ||
| if (datasourceOperations.isConstraintViolation(e)) { | ||
| if (datasourceOperations.isUniquenessConstraintViolation(e)) { |
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
github-project-automation
Bot
moved this from Ready to merge
to Done
in Basic Kanban Board
|
Thanks @snicholasbarton for the fix. Thanks @dimas-b for the review. |
3 participants
Fixes #4553
Currently granting catalog roles with JDBC-backed persistence ends up throwing an HTTP 500 due to a PK constraint violation: all fields in grant records are in the PK and the service endpoint is a PUT (implying idempotency). This change catches the violation and silently succeeds. Later if additional non-PK fields are added to the grant_records table, this implementation might need to change to allow updating those attributes.
I think the alternative here is to propagate conflicts and ultimately throw a 409 - but this contradicts the original Javadoc in BasePersistence, which I think means that the aim is that this does not throw on conflict.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)