8963-globalstorage-improvement-for-sqltools

[danarad05]

Signed-off-by: Dan Arad dan.arad@sap.com

added code improvement for case where extension sends incorrect argument for isGlobal in StorageMain.$set method.

What it does

Fixed #8963

How to test

1. add SQLTOOLS plugin to Theia.
2. close Workspace and open a file
3. close file and no " Cannot save data: no opened workspace" error in terminal.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[paul-marechal]

Do you know what VS Code actually does when requesting to store workspace data but no workspace is opened?

Your PR makes it store data globally when using the workspace storage API and no workspace is opened, does VS Code too?

[danarad05]

Thanks for reviewing @marechal-p. In regard to your comments:

Do you know what VS Code actually does when requesting to store workspace data but no workspace is opened?

As far as I can tell (after debugging VS Code also), workspaceStorage is initialized on startup in VS Code - regardless if a workspace is opened or not.
Both workspaceStorage and globalStorage's folder path derive from User folder:

https://github.com/microsoft/vscode/blob/a699ffaee62010c4634d301da2bbdb7646b8d1da/src/vs/workbench/services/environment/browser/environmentService.ts#L124-L141

In Theia, workspaceStorage is dependent on existence of a workspace.

Your PR makes it store data globally when using the workspace storage API and no workspace is opened, does VS Code too?

Logic for storing globally if workspace doesn't exist, was not implemented by me in this PR: in case it is global storage (isGlobal = true) then it retrieves this.deferredGlobalDataPath. You could argue correctly that now, instead of receiving exception, globalstorage is used - but isn't that what would happen if SQLTools would have sent correct indicator (isGlobal = true - instead of false currently) and so isn't that the wanted behavior in regard to existing workspaceStorage logic?:

theia/packages/plugin-ext/src/main/node/plugins-key-value-storage.ts

Lines 91 to 97 in 6798167

	private async getDataPath(kind: PluginStorageKind): Promise<string | undefined> {
	if (!kind) {
	return this.deferredGlobalDataPath.promise;
	}
	const storagePath = await this.pluginPathsService.getHostStoragePath(kind.workspace, kind.roots);
	return storagePath ? path.join(storagePath, 'workspace-state.json') : undefined;
	}

Debugging VS Code:

[danarad05]

@kittaakos @marechal-p Awaiting your continued review - if possible we would like to enter it to this week' release.
Thanks

[amiramw]

@kittaakos @marechal-p do you think we can merge this?

[kittaakos]

@kittaakos @marechal-p do you think we can merge this?

@amiramw, I am not familiar with this logic to make a decision without further investigation. However, writing into the global storage when no workspaces are opened does not feel good. I debugged into VS Code yesterday, and I can see, the workspaceStorage is indeed initialized although no workspace was opened.

workspaceStorage is initialized on startup in VS Code - regardless if a workspace is opened or not.

In Theia, workspaceStorage is dependent on existence of a workspace.

It seems like this is what we need to do to be VS Code compatible. Cannot you patch/customize theStorageMainImpl on your side for the time being?

[danarad05]

@kittaakos
cc: @amiramw @vince-fugnitto

It seems like this is what we need to do to be VS Code compatible. Cannot you patch/customize the StorageMainImpl on your side for the time being?

First, thanks @kittaakos for reviewing/replying.

Second, in regard to your comment above:
I understand and agree fully that we should direct efforts to making Theia as compatible as possible to VS Code but I'm wondering if that should be done ad-hoc when a bug is found - as this PR is addressing. IMO, it's not supposed to be that while fixing a bug we should implement a change in way Theia is implemented. IMO, ideally a Theia architect should be responsible for laying out the future vision for Theia functionality - the requirements and also not less importantly - their prioritization.

So in short I'm asking why not fix this bug now so that relevant extensions could work properly and add a task to a Theia architect to contemplate future requirements?

I would appreciate also your opinion @vince-fugnitto.

Thanks.

[kittaakos]

why not fix this bug now so that relevant extensions could work properly

With the proposed change, we would write to the global storage when there is no workspace open, and that's the problem, isn't it? So you fix a bug and introduce another.

[danarad05]

why not fix this bug now so that relevant extensions could work properly

With the proposed change, we would write to the global storage when there is no workspace open, and that's the problem, isn't it? So you fix a bug and introduce another.

I don't agree - in current Theia implementation, that is the current logic - if there is a workspace/Storage - write to it, if not use global - that is underlying logic in toKind method. If you compare to VS Code implementation then you might be able to call it "introducing a new bug", but IMO it's not relevant as Theia wasn't and isn't totally implemented in correlation with VS Code. Currently there is an exception - IMO - for the short term solution - the exception should be fixed, for long term solution a requirement should be defined.

@vince-fugnitto ? @amiramw ?

[vince-fugnitto]

Currently there is an exception - IMO - for the short term solution - the exception should be fixed, for long term solution a requirement should be defined.

@vince-fugnitto ? @amiramw ?

I believe I expressed in #8963 the validity of the issue, the expectation that storage be available without a currently opened workspace is incorrect (from what I understand), and while there may be an exception there is no loss of functionality. I'm not sure we should patch our side for an extension which may be doing something incorrect, like the assumption it can set storage at any state.

[danarad05]

Currently there is an exception - IMO - for the short term solution - the exception should be fixed, for long term solution a requirement should be defined.
@vince-fugnitto ? @amiramw ?

I believe I expressed in #8963 the validity of the issue, the expectation that storage be available without a currently opened workspace is incorrect (from what I understand), and while there may be an exception there is no loss of functionality. I'm not sure we should patch our side for an extension which may be doing something incorrect, like the assumption it can set storage at any state.

@vince-fugnitto
Fine. If there is no real issue here or no interest in fixing this issue then I think we should just close it and not waste more time on it.

[danarad05]

closing in favor of #9137.