Fix workspace extensions filer accidentally reading notebooks #1891
Conversation
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| // | ||
| // To read the content of a notebook called `foo` in the workspace the user | ||
| // should use the name with the extension included like `foo.ipynb` or `foo.sql`. | ||
| _, err := w.Stat(ctx, name) |
There was a problem hiding this comment.
Note: In the read-only mode for the filer, this does not introduce an additional network API call since the stat API calls are cached and likely prefetched.
There was a problem hiding this comment.
Thanks.
I notice that the implementation of the underlying filer (the regular workspace files one) also runs a stat on read. It does this to determine that the path is not a directory and that that triggers other behavior. This could be optimized out as well (not needed for this PR).
There was a problem hiding this comment.
Yeah, noticed that as well. Switching over to use_qualified_names should take care of that since we'll no longer have the extensions filer at that point.
| // | ||
| // To read the content of a notebook called `foo` in the workspace the user | ||
| // should use the name with the extension included like `foo.ipynb` or `foo.sql`. | ||
| _, err := w.Stat(ctx, name) |
There was a problem hiding this comment.
Thanks.
I notice that the implementation of the underlying filer (the regular workspace files one) also runs a stat on read. It does this to determine that the path is not a directory and that that triggers other behavior. This could be optimized out as well (not needed for this PR).
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
There was a problem hiding this comment.
FWIW, it is possible to eliminate an extra API call by reusing the stat object in the Read and Delete functions. Once these functions establish that the stat'd path is good, they try to call the parent filer with the original argument, and then call stat again on failure.
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
shreyas-goenka
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
Test Details: go/deco-tests/11895948996 |
**Note:** the `bundle generate` command now uses the `.<resource-type>.yml` sub-extension for the configuration files it writes. Existing configuration files that do not use this sub-extension are renamed to include it. Bundles: * Make `TableName` field part of quality monitor schema ([#1903](#1903)). * Do not prepend paths starting with ~ or variable reference ([#1905](#1905)). * Fix workspace extensions filer accidentally reading notebooks ([#1891](#1891)). * Fix template initialization when running on Databricks ([#1912](#1912)). * Source-linked deployments for bundles in the workspace ([#1884](#1884)). * Added integration test to deploy bundle to /Shared root path ([#1914](#1914)). * Update filenames used by bundle generate to use `.<resource-type>.yml` ([#1901](#1901)). Internal: * Extract functionality to detect if the CLI is running on DBR ([#1889](#1889)). * Consolidate test helpers for `io/fs` ([#1906](#1906)). * Use `fs.FS` interface to read template ([#1910](#1910)). * Use `filer.Filer` to write template instantiation ([#1911](#1911)).
**Note:** the `bundle generate` command now uses the `.<resource-type>.yml` sub-extension for the configuration files it writes. Existing configuration files that do not use this sub-extension are renamed to include it. Bundles: * Make `TableName` field part of quality monitor schema ([#1903](#1903)). * Do not prepend paths starting with ~ or variable reference ([#1905](#1905)). * Fix workspace extensions filer accidentally reading notebooks ([#1891](#1891)). * Fix template initialization when running on Databricks ([#1912](#1912)). * Source-linked deployments for bundles in the workspace ([#1884](#1884)). * Added integration test to deploy bundle to /Shared root path ([#1914](#1914)). * Update filenames used by bundle generate to use `.<resource-type>.yml` ([#1901](#1901)). Internal: * Extract functionality to detect if the CLI is running on DBR ([#1889](#1889)). * Consolidate test helpers for `io/fs` ([#1906](#1906)). * Use `fs.FS` interface to read template ([#1910](#1910)). * Use `filer.Filer` to write template instantiation ([#1911](#1911)).
Changes
The workspace extensions filer should not read or stat a notebook called
fooif the user calls.Stat(ctx, "foo").Instead, the filer should return a file not found error. This is because the contract for the workspace extensions filer is to only work for notebooks when the file path / name includes the extension (example:
foo.ipynborfoo.sqlinstead of justfoo)Tests
Integration tests.