Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement fs save options in exthost notebook. #186805

Merged
merged 1 commit into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/vs/workbench/api/common/extHostFileSystemConsumer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,10 @@ export class ExtHostConsumerFileSystem {
this._fileSystemProvider.set(scheme, { impl: provider, extUri: options?.isCaseSensitive ? extUri : extUriIgnorePathCase, isReadonly: !!options?.isReadonly });
return toDisposable(() => this._fileSystemProvider.delete(scheme));
}

getFileSystemProviderExtUri(scheme: string) {
return this._fileSystemProvider.get(scheme)?.extUri ?? extUri;
}
}

export interface IExtHostConsumerFileSystem extends ExtHostConsumerFileSystem { }
Expand Down
99 changes: 97 additions & 2 deletions src/vs/workbench/api/common/extHostNotebook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { localize } from 'vs/nls';
import { VSBuffer } from 'vs/base/common/buffer';
import { CancellationToken } from 'vs/base/common/cancellation';
import { Emitter, Event } from 'vs/base/common/event';
Expand All @@ -29,8 +30,8 @@ import { ExtHostCell, ExtHostNotebookDocument } from './extHostNotebookDocument'
import { ExtHostNotebookEditor } from './extHostNotebookEditor';
import { onUnexpectedExternalError } from 'vs/base/common/errors';
import { IExtHostConsumerFileSystem } from 'vs/workbench/api/common/extHostFileSystemConsumer';
import { basename } from 'vs/base/common/resources';
import { filter } from 'vs/base/common/objects';
import { Schemas } from 'vs/base/common/network';



Expand Down Expand Up @@ -322,6 +323,17 @@ export class ExtHostNotebookController implements ExtHostNotebookShape {
throw new Error('Document version mismatch');
}

if (!this._extHostFileSystem.value.isWritableFileSystem(uri.scheme)) {
throw new files.FileOperationError(localize('err.readonly', "Unable to modify read-only file '{0}'", this._resourceForError(uri)), files.FileOperationResult.FILE_PERMISSION_DENIED);
}

// validate write
const statBeforeWrite = await this._validateWriteFile(uri, options);

if (!statBeforeWrite) {
await this._mkdirp(uri);
Copy link
Member

Choose a reason for hiding this comment

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

@rebornix writeFile should take care of creating parent folders as needed, any reason you do this anyway?

}

const data: vscode.NotebookData = {
metadata: filter(document.apiNotebook.metadata, key => !(serializer.options?.transientDocumentMetadata ?? {})[key]),
cells: [],
Expand All @@ -344,10 +356,11 @@ export class ExtHostNotebookController implements ExtHostNotebookShape {

const bytes = await serializer.serializer.serializeNotebook(data, token);
await this._extHostFileSystem.value.writeFile(uri, bytes);
const providerExtUri = this._extHostFileSystem.getFileSystemProviderExtUri(uri.scheme);
const stat = await this._extHostFileSystem.value.stat(uri);

const fileStats = {
name: basename(uri), // providerExtUri.basename(resource)
name: providerExtUri.basename(uri),
isFile: (stat.type & files.FileType.File) !== 0,
isDirectory: (stat.type & files.FileType.Directory) !== 0,
isSymbolicLink: (stat.type & files.FileType.SymbolicLink) !== 0,
Expand All @@ -363,6 +376,88 @@ export class ExtHostNotebookController implements ExtHostNotebookShape {
return fileStats;
}

private async _validateWriteFile(uri: URI, options: files.IWriteFileOptions) {
// File system provider registered in Extension Host doesn't have unlock or atomic support
// Validate via file stat meta data
const stat = await this._extHostFileSystem.value.stat(uri);

// File cannot be directory
if ((stat.type & files.FileType.Directory) !== 0) {
throw new files.FileOperationError(localize('fileIsDirectoryWriteError', "Unable to write file '{0}' that is actually a directory", this._resourceForError(uri)), files.FileOperationResult.FILE_IS_DIRECTORY, options);
}

// File cannot be readonly
if ((stat.permissions ?? 0) & files.FilePermission.Readonly) {
throw new files.FileOperationError(localize('err.readonly', "Unable to modify read-only file '{0}'", this._resourceForError(uri)), files.FileOperationResult.FILE_PERMISSION_DENIED);
}

// Dirty write prevention
Copy link
Member

Choose a reason for hiding this comment

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

@rebornix My feeling is that you actually only need this Dirty write prevention, but none of the checks above.

if (
typeof options?.mtime === 'number' && typeof options.etag === 'string' && options.etag !== files.ETAG_DISABLED &&
typeof stat.mtime === 'number' && typeof stat.size === 'number' &&
options.mtime < stat.mtime && options.etag !== files.etag({ mtime: options.mtime /* not using stat.mtime for a reason, see above */, size: stat.size })
) {
throw new files.FileOperationError(localize('fileModifiedError', "File Modified Since"), files.FileOperationResult.FILE_MODIFIED_SINCE, options);
}

return stat;
}

private async _mkdirp(uri: URI) {
const providerExtUri = this._extHostFileSystem.getFileSystemProviderExtUri(uri.scheme);
let directory = providerExtUri.dirname(uri);

const directoriesToCreate: string[] = [];

while (!providerExtUri.isEqual(directory, providerExtUri.dirname(directory))) {
try {
const stat = await this._extHostFileSystem.value.stat(directory);
if ((stat.type & files.FileType.Directory) === 0) {
throw new Error(localize('mkdirExistsError', "Unable to create folder '{0}' that already exists but is not a directory", this._resourceForError(directory)));
}

break; // we have hit a directory that exists -> good
} catch (error) {

// Bubble up any other error that is not file not found
if (files.toFileSystemProviderErrorCode(error) !== files.FileSystemProviderErrorCode.FileNotFound) {
throw error;
}

// Upon error, remember directories that need to be created
directoriesToCreate.push(providerExtUri.basename(directory));

// Continue up
directory = providerExtUri.dirname(directory);
}
}

// Create directories as needed
for (let i = directoriesToCreate.length - 1; i >= 0; i--) {
directory = providerExtUri.joinPath(directory, directoriesToCreate[i]);

try {
await this._extHostFileSystem.value.createDirectory(directory);
} catch (error) {
if (files.toFileSystemProviderErrorCode(error) !== files.FileSystemProviderErrorCode.FileExists) {
// For mkdirp() we tolerate that the mkdir() call fails
// in case the folder already exists. This follows node.js
// own implementation of fs.mkdir({ recursive: true }) and
// reduces the chances of race conditions leading to errors
// if multiple calls try to create the same folders
// As such, we only throw an error here if it is other than
// the fact that the file already exists.
// (see also https://github.com/microsoft/vscode/issues/89834)
throw error;
}
}
}
}

private _resourceForError(uri: URI): string {
return uri.scheme === Schemas.file ? uri.fsPath : uri.toString();
}

// --- open, save, saveAs, backup


Expand Down