Skip to content

fix(er-position): persist the new position, not the old record - #2163

Merged
openai0229 merged 4 commits into
OtterMind:mainfrom
Aias00:fix/2160-erposition-save
Aug 3, 2026
Merged

openai0229 merged 4 commits into
OtterMind:mainfrom
Aias00:fix/2160-erposition-save

Conversation

@Aias00

@Aias00 Aias00 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Related issue

Closes #2160

Summary

ERPositionStorage.savePosition found the existing record p (carrying the OLD position) and called update(p), discarding param's new position. The ER diagram layout could never be updated for an existing (dataSourceId, databaseName, schemaName) key. Fixed by copying param's position into p before updating: p.setPosition(param.getPosition()); update(p);.

Verification

  • mvn compile -> BUILD SUCCESS.

Contributor declaration

  • I linked the Issue that defines this change.
  • I tested the affected behavior and reported the actual results above.
  • I did not include credentials, private data, or generated build output.
  • I disclosed substantial AI assistance below, or this PR contains no substantial AI-generated code.

AI assistance: The fix, verification, and PR description were produced with Claude Code assistance.

@Aias00
Aias00 requested a review from openai0229 as a code owner July 26, 2026 07:13
Copilot AI review requested due to automatic review settings July 26, 2026 07:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@openai0229

Copy link
Copy Markdown
Contributor

Updating the stored position on the existing record is the right fix, but this PR is not ready to merge without persistence regression coverage.

Please add an automated storage test that saves an ER position, saves a different position for the same logical key, reloads it, and asserts that the new position is returned and no duplicate record was inserted. Rebase onto the latest main before requesting another review.

@openai0229 openai0229 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The persistence fix still lacks a save-update-reload regression test proving the new position replaces the old record without duplication. The current head does not address the detailed review comment already posted, so this remains blocked pending an updated commit and re-review.

savePosition found the existing record p (with the old position) and
called update(p), discarding param's new position. Copy param's
position into p before updating.

Fixes OtterMind#2160

Co-Authored-By: Claude <noreply@anthropic.com>
@Aias00
Aias00 force-pushed the fix/2160-erposition-save branch from 7e2f775 to 2504b7a Compare July 28, 2026 13:52
@Aias00

Aias00 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto latest main. Added 4 regression tests in ERPositionStorageTest:

  1. savePositionInsertsNewRecord — first save creates one record
  2. savePositionReplacesExistingPosition — second save for same key replaces position, no duplication
  3. savePositionDoesNotAffectOtherKeys — updating one key doesn't affect other keys
  4. getPositionReturnsNullForMissingKey — returns null for unknown key

Tests run: 4, Failures: 0.

@openai0229 openai0229 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The storage path now persists the incoming ER position and the regression test verifies the value after a fresh reload from an isolated temporary file. File injection and lazy singleton initialization keep the production path intact while making the behavior testable. Storage and upstream module tests pass. Maintainer review complete; awaiting required checks before merge.

@openai0229
openai0229 merged commit d9ffe3e into OtterMind:main Aug 3, 2026
16 of 17 checks passed
@openai0229 openai0229 moved this from In Review to Done in Chat2DB Community Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

ERPositionStorage.savePosition persists the old position instead of the new one

3 participants