-
Notifications
You must be signed in to change notification settings - Fork 237
Conversation
Contributor
Working on adding more tests than just happy path and skeleton assertions, discovered a few problems around SettingsDb/WorkspaceDb versioned lookups and editable cache behavior.
- Fix editable SettingsDb / WorkspaceDb caching so repeated requests reuse the same instance for the same resolved db, with regression coverage for local lookups, semver-equivalent selectors, and default-name/version cases.
- Honor requested versions when creating cloud SettingsDb / WorkspaceDb entries, and default omitted cloud versions to
0.0.0. - Add prototype guarding for
getSetting()usingObject.hasOwn(). - Remove the circular dependency by extracting
SettingsEditorImpl.tsfromSettingsImpl.ts. - Clarify editor docs around local
createEmptyDb(...)path/overwrite behavior and resolved-db caching semantics.
ben-polinsky
reviewed
Mar 18, 2026
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Improves the backend SettingsDb/SettingsEditor implementation to avoid subtle caching and prototype-safety issues, while also restructuring internal code to remove a circular dependency and adding tests to cover the discovered behaviors.
Changes:
- Fixes editable SettingsDb caching so versioned requests don't incorrectly reuse the same cached instance.
- Hardens
SettingsDb.getSettingagainst inherited prototype properties viaObject.hasOwn. - Splits SettingsEditor implementation into
SettingsEditorImpl.tsto break a circular dependency, and adds targeted tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| core/common/src/DbCloudContainerInfo.ts | Strengthens security guidance for baseUri (frontend-safe, no credentials). |
| core/backend/src/workspace/WorkspaceEditor.ts | Adds safety note about file overwrite/path responsibility. |
| core/backend/src/workspace/SettingsEditor.ts | Updates SettingsEditor wiring + docs; clarifies caching intent and adds path safety note. |
| core/backend/src/test/workspace/SettingsDb.test.ts | Adds tests for prototype-guarding and editable DB cache key behavior. |
| core/backend/src/internal/workspace/SettingsImpl.ts | Removes SettingsEditor implementation from this file; exports shared helper. |
| core/backend/src/internal/workspace/SettingsEditorImpl.ts | New extracted SettingsEditor implementation; introduces version-aware caching logic. |
| core/backend/src/internal/workspace/SettingsDbImpl.ts | Adds prototype-guarding in getSetting. |
| common/changes/@itwin/core-common/nam-settingsDb-improvements_2026-03-18-16-16.json | Change record for core-common. |
| common/changes/@itwin/core-backend/nam-settingsDb-improvements_2026-03-18-16-16.json | Change record for core-backend. |
| common/api/core-backend.api.md | API report update for added documentation on EditableSettingsDb.container. |
ben-polinsky
reviewed
Mar 18, 2026
johnnyd710
reviewed
Mar 18, 2026
johnnyd710
reviewed
Mar 18, 2026
johnnyd710
approved these changes
Mar 19, 2026
ben-polinsky
reviewed
Mar 19, 2026
common/changes/@itwin/core-backend/nam-settingsDb-improvements_2026-03-18-16-16.json
Outdated
Show resolved
Hide resolved
ben-polinsky
approved these changes
Mar 19, 2026
ben-polinsky
approved these changes
Mar 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.