Merged
Conversation
When re-importing a test database, if the source archive for that database is not in the workspace, then old source code will be seen when inspected. To reproduce: 1. Run a ql test file with a failure (I'm using a javascript DB). 2. Right click and import on the db that sticks around. 3. Change the JS source file for the test. 4. Re-run and still have a failure. 5. Re-import the database 6. Run the query under test 7. Click on a result item 8. **BUG:** The source code is old The problem is that the source archive cache is not being flushed in this case. I added a case to flush the source archive when the archive was imported into the workspace as a folder, but not when the archive is external. The fix is to listen for database remove events in the archive filesystem provider and flush the associated database source archive. There is a complication: The database remove event didn't emit the removed database. This is because the only place that consumed the event didn't need it. To get around this, I changed the structure of the events. I added a new `fullRefresh` boolean. If true, then the original database change handler will ensure the entire tree is refreshed. If false, only the selected database.
aeisenberg
commented
May 17, 2024
Comment on lines
+29
to
+30
| import { DatabaseEventKind } from "../../databases/local-databases/database-events"; | ||
| import type { DatabaseManager } from "../../databases/local-databases/database-manager"; |
Contributor
Author
There was a problem hiding this comment.
I had to do this in order to avoid cyclic import statements.
| // note that we use undefined as the item in order to reset the entire tree | ||
| this._onDidChangeDatabaseItem.fire({ | ||
| item: undefined, | ||
| item, |
Contributor
Author
There was a problem hiding this comment.
There is actually nowhere that listens to the add event and requires this value to be defined. Just for consistency, I thought I'd change it.
koesie10
reviewed
May 21, 2024
Member
koesie10
left a comment
There was a problem hiding this comment.
Code changes LGTM, but it seems like the tests still need to be updated
extensions/ql-vscode/src/databases/local-databases/database-events.ts
Outdated
Show resolved
Hide resolved
extensions/ql-vscode/src/databases/local-databases/database-manager.ts
Outdated
Show resolved
Hide resolved
extensions/ql-vscode/src/common/vscode/archive-filesystem-provider.ts
Outdated
Show resolved
Hide resolved
Also: - Address comments in PR - Add changelog note
a3609d6 to
088d2fa
Compare
koesie10
approved these changes
May 23, 2024
extensions/ql-vscode/src/databases/local-databases/database-events.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Koen Vlaswinkel <koesie10@users.noreply.github.com>
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
2 participants
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.
When re-importing a test database, if the source archive for that database is not in the workspace, then old source code will be seen when inspected.
To reproduce:
The problem is that the source archive cache is not being flushed in this case. I added a case to flush the source archive when the archive was imported into the workspace as a folder, but not when the archive is external.
The fix is to listen for database remove events in the archive filesystem provider and flush the associated database source archive.
There is a complication:
The database remove event didn't emit the removed database. This is because the only place that consumed the event didn't need it.
To get around this, I changed the structure of the events. I added a new
fullRefreshboolean. If true, then the original database change handler will ensure the entire tree is refreshed. If false, only the selected database.Replace this with a description of the changes your pull request makes.
Checklist
ready-for-doc-reviewlabel there.