You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "kimyenac (via GitHub)" <gi...@apache.org> on 2026/10/08 06:11:15 UTC
[PR] [ZEPPELIN-6736] Implement Shared Notebook Core read state and selectors [zeppelin]
kimyenac opened a new pull request, #5559:
URL: https://github.com/apache/zeppelin/pull/5559
### What is this PR for?
Implement the read side of the Shared Notebook Core: plain TypeScript state, a store with cached immutable snapshots, and selectors. Everything is in `projects/zeppelin-notebook-core`, with no SDK, REST, WebSocket, RxJS or framework import. The existing boundary check enforces that: the Core may import only its own sources and `tslib`. Connecting a host transport and an Angular consumer is ZEPPELIN-6737.
Commits:
1. **Read inputs and read state** (`read-model.ts`):
* Core-owned input types for the NOTE note, NOTE_REVISION and the note permissions REST response. The Core cannot import SDK types, so these copy the shapes it reads.
* `test/notebook-core/read-input-compat.spec.ts` checks at compile time that the SDK `Note` / `NoteRevision` and the app `Permissions` types still pass into them unchanged.
* The read snapshot extends `NotebookCoreSnapshot`, so a read store can be handed to a remote as its `NotebookPort`.
2. **Read store** (`read-store.ts`):
* `activate(target)` returns a request handle; the host passes each response through it.
* Snapshots are deeply frozen copies of the input. Unchanged branches keep their identity.
* Notifications are deterministic, and `dispose` cleans up.
3. **Selectors** (`read-selectors.ts`): ordered paragraphs, a paragraph and its persisted chart settings, and note capabilities.
#### Read state
* **Target:** `{ kind: 'note', noteId }` or `{ kind: 'revision', noteId, revisionId }`. A live note and a revision of it are distinct targets.
* **Status:** `idle`, `loading`, `ready`, `not-found`, `access-denied`, `failed`, `disposed`.
* `not-found` and `access-denied` are reported only when the host can tell them apart.
* A failure ends the request, and a failure after `ready` is rejected.
* **Note:** `id`, `name`, `path`, `paragraphIds` (order) and `paragraphsById`. Each paragraph holds `id`, `title`, `text`, `status`, `resultCode`, `resultMessages` and `chartConfigs` (persisted `graph` settings by result index, kept uninterpreted).
* **Permissions:** `unknown`, `loading`, `ready`, `access-denied`, `failed`, plus the four lists.
* **Capabilities** (`selectAclCapabilities`) follow `AuthorizationService`'s list rules:
* an empty list admits everyone;
* owners can write, writers can run, runners can read;
* nothing is granted until the lists are `ready`.
The server's admin role and anonymous mode are not visible to the Core. So this can deny what the server allows, but never allows what the lists deny.
#### Read operation inventory
| Operation | Core input | Included |
| --- | --- | --- |
| `GET_NOTE` → `NOTE` (full note) | `request.acceptNote(note)` | yes |
| `NOTE_REVISION` (full revision) | `request.acceptRevision(revision)`; a response without a note is `not-found` | yes |
| `GET /api/notebook/{id}/permissions` | `request.readAcl()`, then `accept(lists)` or `fail(reason)` | yes |
| `ERROR_INFO` / `AUTH_INFO` for a load | `request.fail(reason)`, only while loading | as an outcome the host attributes |
| `PARAGRAPH`, `PATCH_PARAGRAPH`, `PARAGRAPH_ADDED` / `REMOVED` / `MOVED`, `NOTE_UPDATED`, progress, streaming output, collaboration, angular objects | none | **no** |
**Reload policy.** The excluded events are not applied as live updates. Read state changes only through a full `NOTE` or `NOTE_REVISION`, or a new `activate()`. A host that needs current state re-activates the target or delivers a full `NOTE`. When a later full `NOTE` arrives for the same target, it replaces the loaded note, and unchanged paragraphs keep their identity.
#### Field ownership
* **Server (Core-owned read state):** note identity and name/path, paragraph order and content, saved results, persisted chart settings, revision identity, and the permission lists.
* **Host (outside the Core):** authentication and the principal, navigation and route parameters, server authorization (the admin role and anonymous mode), the transport and request attribution.
* **Not held:** editor drafts, run/cancel/save state and live execution output (later sub-tasks of ZEPPELIN-6687).
#### Attribution: what the wire provides and what remains unresolved
Checked against `NotebookServer` and `WebSocketServiceCallback` on `master`:
* **`NOTE` has no `msgId`.** The same op is sent for `GET_NOTE`, `RELOAD_NOTE` and the note-change broadcast (`inlineBroadcastNote`). On the wire, a `NOTE` can be attributed only by `note.id`, never to a request.
* **Unresolved:** in A → B → A, a late `NOTE(A)` from the first visit is indistinguishable from the second visit's response.
* The Core rejects the first visit's handle. But a host that routes `NOTE` by note id alone would deliver that late `NOTE(A)` through the second visit's handle.
* As the issue notes, stamping the current generation when the message arrives does not establish its origin.
* **`NOTE_REVISION` has no `msgId`** but carries `noteId` and `revisionId`. A repeated request for the same revision is likewise indistinguishable on the wire. Its content is the same immutable revision, so accepting either response gives the same state.
* **Load failures:** `ERROR_INFO` (not found or other) and `AUTH_INFO` (forbidden) carry only an `info` string, with no note id or `msgId`. The op tells the kind apart, but not which request failed.
* **Permissions:** a REST response is bound to its request, so its handle attribution is exact.
In the specs, the request handles are a **synthetic request context**, and the `request handles` describe block says so. They verify the Core's contract for responses the host has attributed. They are not evidence that the current wire can attribute them. That is ZEPPELIN-6737's acceptance criterion, as is any minimal read-transport change it needs.
**Not in this PR:** route wiring, Angular or React rendering, editor drafts, save/run/cancel commands, structural mutations, incremental output, collaboration, reconnect recovery, ACL writes and React opt-in. This does not complete ZEPPELIN-6687.
### What type of PR is it?
Improvement
### Todos
* [x] Core-owned read inputs, with a compile-time SDK compatibility check
* [x] Read store: request handles, frozen snapshots, stable identity, deterministic notifications, dispose
* [x] Selectors for ordered paragraphs, results, chart settings and capabilities
* [x] Read operation inventory, field ownership and unresolved attribution cases (above)
### What is the Jira issue?
[ZEPPELIN-6736](https://issues.apache.org/jira/browse/ZEPPELIN-6736)
### How should this be tested?
```bash
cd zeppelin-web-angular
npm run test:notebook-core # 159 passed (45 new store/selector specs, 1 compat spec)
npm run typecheck:notebook-core # passes (includes the compat spec and the @ts-expect-error mutation checks)
npm run typecheck:react # passes
npm run test:react # passes
npx eslint projects/zeppelin-notebook-core/src test/notebook-core
```
No Angular, React or browser is needed. Each of the three commits passes `test:notebook-core`, the Core typecheck and eslint on its own (115 → 145 → 159 tests).
Covered:
* **Load outcomes:** initial load, `not-found`, `access-denied`, `failed`, a failure ending the request, and no failure after `ready`.
* **Targets:** live note ↔ revision transitions, A → B → A, the same revision requested again, mismatched note or revision ids, and deactivate.
* **Snapshots:**
* the same reference while nothing changes, including for an identical full `NOTE`;
* paragraph and order identity on partial changes;
* no reference kept to host input;
* nested mutation throws a `TypeError` (and is rejected by the type checker), including the permission lists in every permissions state.
* **Subscriptions:** notification order, subscribing and unsubscribing while notifying, the same listener subscribed twice, and dispose.
* **Permissions:** reads, failures, a replaced read, a read from a replaced activation, and list identity across re-reads.
**Each spec catches the defect it targets.** I broke the implementation on purpose and checked that the failures were assertions:
* a handle that ignores its own currency;
* no paragraph reuse;
* no freezing;
* no note id check;
* an empty list treated as closed;
* capabilities granted while permissions are unknown;
* in-place subscription changes;
* shallow-frozen permission lists.
Breaking the compat types fails `typecheck:notebook-core` for all three payloads.
### Screenshots (if appropriate)
N/A.
### Questions:
* Does the license files need to update? No. New files carry the ASF header.
* Is there breaking changes for older versions? No. The existing contract types are unchanged; the public API only gains exports.
* Does this needs documentation? The read model is documented in the source and in this PR.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6736] Implement Shared Notebook Core read state and selectors [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on PR #5559:
URL: https://github.com/apache/zeppelin/pull/5559#issuecomment-6105432664
Merged into master (f9754f98).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6736] Implement Shared Notebook Core read state and selectors [zeppelin]
Posted by "kimyenac (via GitHub)" <gi...@apache.org>.
kimyenac commented on code in PR #5559:
URL: https://github.com/apache/zeppelin/pull/5559#discussion_r4226998919
##########
zeppelin-web-angular/projects/zeppelin-notebook-core/src/read-store.ts:
##########
@@ -0,0 +1,346 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import type {
+ NotebookAclInput,
+ NotebookAclStatus,
+ NotebookNoteInput,
+ NotebookParagraphInput,
+ NotebookReadAcl,
+ NotebookReadFailure,
+ NotebookReadNote,
+ NotebookReadParagraph,
+ NotebookReadSnapshot,
+ NotebookReadStatus,
+ NotebookReadTarget,
+ NotebookRevisionInput
+} from './read-model';
+
+/**
+ * The lifetime of one permissions read. Responses are accepted only while this is the latest
+ * permissions read of the current activation.
+ */
+export interface NotebookAclRequest {
+ isCurrent(): boolean;
+ accept(acl: NotebookAclInput): boolean;
+ fail(reason: 'access-denied' | 'failed'): boolean;
+}
+
+/**
+ * The lifetime of one activation. The host keeps this handle with the request it sends and passes
+ * the response through it. Once another activation, a deactivation or dispose replaces it, every
+ * method rejects, including for the same target visited again.
+ *
+ * Each method returns whether the input was applied.
+ */
+export interface NotebookReadRequest {
+ readonly target: NotebookReadTarget;
+ isCurrent(): boolean;
+ /** A full NOTE for a live-note target. Accepted while loading or ready. */
+ acceptNote(note: NotebookNoteInput): boolean;
+ /** A full NOTE_REVISION for a revision target. A response without a note is `not-found`. */
+ acceptRevision(revision: NotebookRevisionInput): boolean;
+ /** Ends a load that has not completed. A failure ends this request; activate again to retry. */
+ fail(reason: NotebookReadFailure): boolean;
+ /** Starts a permissions read. A later call replaces an earlier one. */
+ readAcl(): NotebookAclRequest;
+}
+
+export interface NotebookReadStore {
+ /** Returns the same reference until the read state changes. */
+ getSnapshot(): NotebookReadSnapshot;
+ /** Listeners run in subscription order, once per snapshot change. */
+ subscribe(listener: () => void): () => void;
+ activate(target: NotebookReadTarget): NotebookReadRequest;
+ deactivate(): void;
+ /** Publishes a final `disposed` snapshot, then drops every listener. */
+ dispose(): void;
+}
+
+// Read state is built from wire data: null, booleans, numbers, strings, arrays and plain objects.
+const freezeCopy = (value: unknown): unknown => {
+ if (Array.isArray(value)) {
+ return Object.freeze(value.map(freezeCopy));
+ }
+ if (value !== null && typeof value === 'object') {
+ const copy: { [key: string]: unknown } = {};
+ for (const key of Object.keys(value)) {
+ copy[key] = freezeCopy((value as { [key: string]: unknown })[key]);
+ }
+ return Object.freeze(copy);
+ }
+ return value;
+};
+
+const sameValue = (a: unknown, b: unknown): boolean => {
+ if (a === b) {
+ return true;
+ }
+ if (Array.isArray(a) && Array.isArray(b)) {
+ return a.length === b.length && a.every((item, index) => sameValue(item, b[index]));
+ }
+ if (a !== null && b !== null && typeof a === 'object' && typeof b === 'object') {
+ if (Array.isArray(a) || Array.isArray(b)) {
+ return false;
+ }
+ const aKeys = Object.keys(a);
+ const bKeys = Object.keys(b);
+ return (
+ aKeys.length === bKeys.length &&
+ aKeys.every(
+ key =>
+ Object.prototype.hasOwnProperty.call(b, key) &&
+ sameValue((a as { [key: string]: unknown })[key], (b as { [key: string]: unknown })[key])
+ )
+ );
+ }
+ return false;
+};
+
+/** Keeps the previous object when the next one has the same content. */
+const reuse = <T>(previous: T | undefined, next: T): T =>
+ previous !== undefined && sameValue(previous, next) ? previous : next;
+
+const emptyAcl = (status: NotebookAclStatus): NotebookReadAcl =>
+ freezeCopy({ status, owners: [], readers: [], runners: [], writers: [] }) as NotebookReadAcl;
+
+const unknownAcl = emptyAcl('unknown');
+const loadingAcl = emptyAcl('loading');
+
+const buildParagraph = (input: NotebookParagraphInput): NotebookReadParagraph => {
+ const chartConfigs: { [resultIndex: string]: unknown } = {};
+ const configResults = input.config?.results ?? {};
+ for (const resultIndex of Object.keys(configResults)) {
+ const graph = configResults[resultIndex]?.graph;
+ if (graph !== undefined) {
+ chartConfigs[resultIndex] = graph;
+ }
+ }
+ return freezeCopy({
+ id: input.id,
+ title: input.title ?? null,
+ text: input.text ?? '',
+ status: input.status,
+ resultCode: input.results?.code ?? null,
+ resultMessages: (input.results?.msg ?? []).map(message => ({ type: message.type, data: message.data })),
+ chartConfigs
+ }) as NotebookReadParagraph;
+};
+
+const buildNote = (input: NotebookNoteInput, previous: NotebookReadNote | null): NotebookReadNote => {
+ const paragraphsById: { [paragraphId: string]: NotebookReadParagraph } = {};
Review Comment:
Fixed in 9334f29ea. Read-state keys are now defined as own properties (`setOwn`) in `paragraphsById`, `chartConfigs` and the deep copy. New specs cover a `__proto__` paragraph that a second full `NOTE` updates, `__proto__` keys inside chart settings, and selector lookup. All of them fail with the previous assignment code.
##########
zeppelin-web-angular/projects/zeppelin-notebook-core/src/read-store.ts:
##########
@@ -0,0 +1,346 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import type {
+ NotebookAclInput,
+ NotebookAclStatus,
+ NotebookNoteInput,
+ NotebookParagraphInput,
+ NotebookReadAcl,
+ NotebookReadFailure,
+ NotebookReadNote,
+ NotebookReadParagraph,
+ NotebookReadSnapshot,
+ NotebookReadStatus,
+ NotebookReadTarget,
+ NotebookRevisionInput
+} from './read-model';
+
+/**
+ * The lifetime of one permissions read. Responses are accepted only while this is the latest
+ * permissions read of the current activation.
+ */
+export interface NotebookAclRequest {
+ isCurrent(): boolean;
+ accept(acl: NotebookAclInput): boolean;
+ fail(reason: 'access-denied' | 'failed'): boolean;
+}
+
+/**
+ * The lifetime of one activation. The host keeps this handle with the request it sends and passes
+ * the response through it. Once another activation, a deactivation or dispose replaces it, every
+ * method rejects, including for the same target visited again.
+ *
+ * Each method returns whether the input was applied.
+ */
+export interface NotebookReadRequest {
+ readonly target: NotebookReadTarget;
+ isCurrent(): boolean;
+ /** A full NOTE for a live-note target. Accepted while loading or ready. */
+ acceptNote(note: NotebookNoteInput): boolean;
+ /** A full NOTE_REVISION for a revision target. A response without a note is `not-found`. */
+ acceptRevision(revision: NotebookRevisionInput): boolean;
+ /** Ends a load that has not completed. A failure ends this request; activate again to retry. */
+ fail(reason: NotebookReadFailure): boolean;
+ /** Starts a permissions read. A later call replaces an earlier one. */
+ readAcl(): NotebookAclRequest;
+}
+
+export interface NotebookReadStore {
+ /** Returns the same reference until the read state changes. */
+ getSnapshot(): NotebookReadSnapshot;
+ /** Listeners run in subscription order, once per snapshot change. */
+ subscribe(listener: () => void): () => void;
+ activate(target: NotebookReadTarget): NotebookReadRequest;
+ deactivate(): void;
+ /** Publishes a final `disposed` snapshot, then drops every listener. */
+ dispose(): void;
+}
+
+// Read state is built from wire data: null, booleans, numbers, strings, arrays and plain objects.
+const freezeCopy = (value: unknown): unknown => {
+ if (Array.isArray(value)) {
+ return Object.freeze(value.map(freezeCopy));
+ }
+ if (value !== null && typeof value === 'object') {
+ const copy: { [key: string]: unknown } = {};
+ for (const key of Object.keys(value)) {
+ copy[key] = freezeCopy((value as { [key: string]: unknown })[key]);
+ }
+ return Object.freeze(copy);
+ }
+ return value;
+};
+
+const sameValue = (a: unknown, b: unknown): boolean => {
+ if (a === b) {
+ return true;
+ }
+ if (Array.isArray(a) && Array.isArray(b)) {
+ return a.length === b.length && a.every((item, index) => sameValue(item, b[index]));
+ }
+ if (a !== null && b !== null && typeof a === 'object' && typeof b === 'object') {
+ if (Array.isArray(a) || Array.isArray(b)) {
+ return false;
+ }
+ const aKeys = Object.keys(a);
+ const bKeys = Object.keys(b);
+ return (
+ aKeys.length === bKeys.length &&
+ aKeys.every(
+ key =>
+ Object.prototype.hasOwnProperty.call(b, key) &&
+ sameValue((a as { [key: string]: unknown })[key], (b as { [key: string]: unknown })[key])
+ )
+ );
+ }
+ return false;
+};
+
+/** Keeps the previous object when the next one has the same content. */
+const reuse = <T>(previous: T | undefined, next: T): T =>
+ previous !== undefined && sameValue(previous, next) ? previous : next;
+
+const emptyAcl = (status: NotebookAclStatus): NotebookReadAcl =>
+ freezeCopy({ status, owners: [], readers: [], runners: [], writers: [] }) as NotebookReadAcl;
+
+const unknownAcl = emptyAcl('unknown');
+const loadingAcl = emptyAcl('loading');
+
+const buildParagraph = (input: NotebookParagraphInput): NotebookReadParagraph => {
+ const chartConfigs: { [resultIndex: string]: unknown } = {};
+ const configResults = input.config?.results ?? {};
+ for (const resultIndex of Object.keys(configResults)) {
+ const graph = configResults[resultIndex]?.graph;
+ if (graph !== undefined) {
+ chartConfigs[resultIndex] = graph;
+ }
+ }
+ return freezeCopy({
+ id: input.id,
+ title: input.title ?? null,
+ text: input.text ?? '',
+ status: input.status,
+ resultCode: input.results?.code ?? null,
+ resultMessages: (input.results?.msg ?? []).map(message => ({ type: message.type, data: message.data })),
+ chartConfigs
+ }) as NotebookReadParagraph;
+};
+
+const buildNote = (input: NotebookNoteInput, previous: NotebookReadNote | null): NotebookReadNote => {
+ const paragraphsById: { [paragraphId: string]: NotebookReadParagraph } = {};
+ for (const paragraphInput of input.paragraphs) {
+ const previousParagraph =
+ previous && Object.prototype.hasOwnProperty.call(previous.paragraphsById, paragraphInput.id)
+ ? previous.paragraphsById[paragraphInput.id]
+ : undefined;
+ paragraphsById[paragraphInput.id] = reuse(previousParagraph, buildParagraph(paragraphInput));
+ }
+ const next: NotebookReadNote = Object.freeze({
+ id: input.id,
+ name: input.name,
+ path: input.path ?? null,
+ paragraphIds: reuse(previous?.paragraphIds, Object.freeze(input.paragraphs.map(paragraph => paragraph.id))),
+ paragraphsById: Object.freeze(paragraphsById)
+ });
+ return reuse(previous ?? undefined, next);
+};
+
+const makeSnapshot = (
+ target: NotebookReadTarget | null,
+ status: NotebookReadStatus,
+ note: NotebookReadNote | null,
+ acl: NotebookReadAcl
+): NotebookReadSnapshot =>
+ Object.freeze({
+ noteId: target?.noteId ?? '',
+ revisionId: target?.kind === 'revision' ? target.revisionId : null,
+ target,
+ status,
+ note,
+ acl
+ });
+
+const idleSnapshot = makeSnapshot(null, 'idle', null, unknownAcl);
+const disposedSnapshot = makeSnapshot(null, 'disposed', null, unknownAcl);
+
+const staleAclRequest: NotebookAclRequest = Object.freeze({
+ isCurrent: () => false,
+ accept: () => false,
+ fail: () => false
+});
+
+export const createNotebookReadStore = (): NotebookReadStore => {
+ let snapshot = idleSnapshot;
+ // One entry per subscription, so subscribing the same function twice needs two unsubscribes.
+ let subscriptions: Array<{ listener: () => void }> = [];
+ // Identity tokens for the current activation and permissions read. A handle compares its own
+ // token with these, so a replaced handle stays rejected even when the same target returns.
+ let activeRequest: object | null = null;
+ let activeAclRequest: object | null = null;
+ let disposed = false;
+
+ const publish = (next: NotebookReadSnapshot): void => {
+ if (next === snapshot) {
+ return;
+ }
+ snapshot = next;
+ // subscribe and unsubscribe replace the array, so changes made while notifying take effect
+ // from the next change.
+ for (const subscription of subscriptions) {
+ subscription.listener();
+ }
+ };
+
+ // Changes the read state of the current target; activation publishes a new target directly.
+ const update = (changes: Partial<Pick<NotebookReadSnapshot, 'status' | 'note' | 'acl'>>): void => {
+ const status = changes.status ?? snapshot.status;
+ const note = 'note' in changes ? (changes.note ?? null) : snapshot.note;
+ const acl = changes.acl ?? snapshot.acl;
+ if (status === snapshot.status && note === snapshot.note && acl === snapshot.acl) {
+ return;
+ }
+ publish(makeSnapshot(snapshot.target, status, note, acl));
+ };
+
+ const activate = (requestedTarget: NotebookReadTarget): NotebookReadRequest => {
+ if (disposed) {
+ throw new Error('Cannot activate a disposed notebook read store');
+ }
+ const token = {};
+ activeRequest = token;
+ activeAclRequest = null;
+ const target = freezeCopy(requestedTarget) as NotebookReadTarget;
+ publish(makeSnapshot(target, 'loading', null, unknownAcl));
+
+ const isCurrent = (): boolean => activeRequest === token;
+ const canLoad = (): boolean => isCurrent() && (snapshot.status === 'loading' || snapshot.status === 'ready');
+
+ // A permissions re-read passes through `loading`; reuse the last lists read in this activation.
+ let lastReadyAcl: NotebookReadAcl | undefined;
+
+ const acceptFullNote = (input: NotebookNoteInput): void => {
+ const previous = snapshot.status === 'ready' ? snapshot.note : null;
+ update({ status: 'ready', note: buildNote(input, previous) });
+ };
+
+ const readAcl = (): NotebookAclRequest => {
+ if (!isCurrent()) {
+ return staleAclRequest;
+ }
+ const aclToken = {};
+ activeAclRequest = aclToken;
+ update({ acl: loadingAcl });
+ const isAclCurrent = (): boolean => isCurrent() && activeAclRequest === aclToken;
+ return Object.freeze({
+ isCurrent: isAclCurrent,
+ accept: (input: NotebookAclInput): boolean => {
+ if (!isAclCurrent()) {
+ return false;
+ }
+ const acl = freezeCopy({
+ status: 'ready',
+ owners: input.owners,
+ readers: input.readers,
+ runners: input.runners,
+ writers: input.writers
+ }) as NotebookReadAcl;
+ lastReadyAcl = reuse(lastReadyAcl, acl);
+ update({ acl: lastReadyAcl });
+ return true;
+ },
+ fail: (reason: 'access-denied' | 'failed'): boolean => {
+ if (!isAclCurrent()) {
+ return false;
+ }
+ update({ acl: emptyAcl(reason) });
+ return true;
+ }
+ });
+ };
+
+ return Object.freeze({
+ target,
+ isCurrent,
+ acceptNote: (input: NotebookNoteInput): boolean => {
+ if (!canLoad() || target.kind !== 'note' || input.id !== target.noteId) {
+ return false;
+ }
+ acceptFullNote(input);
+ return true;
+ },
+ acceptRevision: (input: NotebookRevisionInput): boolean => {
+ if (
+ !canLoad() ||
+ target.kind !== 'revision' ||
+ input.noteId !== target.noteId ||
+ input.revisionId !== target.revisionId
+ ) {
+ return false;
+ }
+ if (input.note === undefined || input.note === null) {
+ if (snapshot.status !== 'loading') {
+ return false;
+ }
+ update({ status: 'not-found', note: null });
Review Comment:
Fixed in 9334f29ea. A `NOTE_REVISION` without a note now reports `failed`, and the code, spec and the inventory row in the PR description note that `getNoteByRevision` returns none for both a missing revision and an unsupported repository.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6736] Implement Shared Notebook Core read state and selectors [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha merged PR #5559:
URL: https://github.com/apache/zeppelin/pull/5559
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6736] Implement Shared Notebook Core read state and selectors [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on code in PR #5559:
URL: https://github.com/apache/zeppelin/pull/5559#discussion_r4219878155
##########
zeppelin-web-angular/projects/zeppelin-notebook-core/src/read-store.ts:
##########
@@ -0,0 +1,346 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import type {
+ NotebookAclInput,
+ NotebookAclStatus,
+ NotebookNoteInput,
+ NotebookParagraphInput,
+ NotebookReadAcl,
+ NotebookReadFailure,
+ NotebookReadNote,
+ NotebookReadParagraph,
+ NotebookReadSnapshot,
+ NotebookReadStatus,
+ NotebookReadTarget,
+ NotebookRevisionInput
+} from './read-model';
+
+/**
+ * The lifetime of one permissions read. Responses are accepted only while this is the latest
+ * permissions read of the current activation.
+ */
+export interface NotebookAclRequest {
+ isCurrent(): boolean;
+ accept(acl: NotebookAclInput): boolean;
+ fail(reason: 'access-denied' | 'failed'): boolean;
+}
+
+/**
+ * The lifetime of one activation. The host keeps this handle with the request it sends and passes
+ * the response through it. Once another activation, a deactivation or dispose replaces it, every
+ * method rejects, including for the same target visited again.
+ *
+ * Each method returns whether the input was applied.
+ */
+export interface NotebookReadRequest {
+ readonly target: NotebookReadTarget;
+ isCurrent(): boolean;
+ /** A full NOTE for a live-note target. Accepted while loading or ready. */
+ acceptNote(note: NotebookNoteInput): boolean;
+ /** A full NOTE_REVISION for a revision target. A response without a note is `not-found`. */
+ acceptRevision(revision: NotebookRevisionInput): boolean;
+ /** Ends a load that has not completed. A failure ends this request; activate again to retry. */
+ fail(reason: NotebookReadFailure): boolean;
+ /** Starts a permissions read. A later call replaces an earlier one. */
+ readAcl(): NotebookAclRequest;
+}
+
+export interface NotebookReadStore {
+ /** Returns the same reference until the read state changes. */
+ getSnapshot(): NotebookReadSnapshot;
+ /** Listeners run in subscription order, once per snapshot change. */
+ subscribe(listener: () => void): () => void;
+ activate(target: NotebookReadTarget): NotebookReadRequest;
+ deactivate(): void;
+ /** Publishes a final `disposed` snapshot, then drops every listener. */
+ dispose(): void;
+}
+
+// Read state is built from wire data: null, booleans, numbers, strings, arrays and plain objects.
+const freezeCopy = (value: unknown): unknown => {
+ if (Array.isArray(value)) {
+ return Object.freeze(value.map(freezeCopy));
+ }
+ if (value !== null && typeof value === 'object') {
+ const copy: { [key: string]: unknown } = {};
+ for (const key of Object.keys(value)) {
+ copy[key] = freezeCopy((value as { [key: string]: unknown })[key]);
+ }
+ return Object.freeze(copy);
+ }
+ return value;
+};
+
+const sameValue = (a: unknown, b: unknown): boolean => {
+ if (a === b) {
+ return true;
+ }
+ if (Array.isArray(a) && Array.isArray(b)) {
+ return a.length === b.length && a.every((item, index) => sameValue(item, b[index]));
+ }
+ if (a !== null && b !== null && typeof a === 'object' && typeof b === 'object') {
+ if (Array.isArray(a) || Array.isArray(b)) {
+ return false;
+ }
+ const aKeys = Object.keys(a);
+ const bKeys = Object.keys(b);
+ return (
+ aKeys.length === bKeys.length &&
+ aKeys.every(
+ key =>
+ Object.prototype.hasOwnProperty.call(b, key) &&
+ sameValue((a as { [key: string]: unknown })[key], (b as { [key: string]: unknown })[key])
+ )
+ );
+ }
+ return false;
+};
+
+/** Keeps the previous object when the next one has the same content. */
+const reuse = <T>(previous: T | undefined, next: T): T =>
+ previous !== undefined && sameValue(previous, next) ? previous : next;
+
+const emptyAcl = (status: NotebookAclStatus): NotebookReadAcl =>
+ freezeCopy({ status, owners: [], readers: [], runners: [], writers: [] }) as NotebookReadAcl;
+
+const unknownAcl = emptyAcl('unknown');
+const loadingAcl = emptyAcl('loading');
+
+const buildParagraph = (input: NotebookParagraphInput): NotebookReadParagraph => {
+ const chartConfigs: { [resultIndex: string]: unknown } = {};
+ const configResults = input.config?.results ?? {};
+ for (const resultIndex of Object.keys(configResults)) {
+ const graph = configResults[resultIndex]?.graph;
+ if (graph !== undefined) {
+ chartConfigs[resultIndex] = graph;
+ }
+ }
+ return freezeCopy({
+ id: input.id,
+ title: input.title ?? null,
+ text: input.text ?? '',
+ status: input.status,
+ resultCode: input.results?.code ?? null,
+ resultMessages: (input.results?.msg ?? []).map(message => ({ type: message.type, data: message.data })),
+ chartConfigs
+ }) as NotebookReadParagraph;
+};
+
+const buildNote = (input: NotebookNoteInput, previous: NotebookReadNote | null): NotebookReadNote => {
+ const paragraphsById: { [paragraphId: string]: NotebookReadParagraph } = {};
Review Comment:
An imported note can preserve a paragraph ID such as `__proto__`. Assigning that ID into a plain object does not create an own property, so paragraph lookup and a subsequent full `NOTE` update can silently behave incorrectly. Could we preserve special keys in `paragraphsById`, `chartConfigs`, and the deep-copy path, and add a regression test that updates the same ID with a second full `NOTE`?
##########
zeppelin-web-angular/projects/zeppelin-notebook-core/src/read-store.ts:
##########
@@ -0,0 +1,346 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import type {
+ NotebookAclInput,
+ NotebookAclStatus,
+ NotebookNoteInput,
+ NotebookParagraphInput,
+ NotebookReadAcl,
+ NotebookReadFailure,
+ NotebookReadNote,
+ NotebookReadParagraph,
+ NotebookReadSnapshot,
+ NotebookReadStatus,
+ NotebookReadTarget,
+ NotebookRevisionInput
+} from './read-model';
+
+/**
+ * The lifetime of one permissions read. Responses are accepted only while this is the latest
+ * permissions read of the current activation.
+ */
+export interface NotebookAclRequest {
+ isCurrent(): boolean;
+ accept(acl: NotebookAclInput): boolean;
+ fail(reason: 'access-denied' | 'failed'): boolean;
+}
+
+/**
+ * The lifetime of one activation. The host keeps this handle with the request it sends and passes
+ * the response through it. Once another activation, a deactivation or dispose replaces it, every
+ * method rejects, including for the same target visited again.
+ *
+ * Each method returns whether the input was applied.
+ */
+export interface NotebookReadRequest {
+ readonly target: NotebookReadTarget;
+ isCurrent(): boolean;
+ /** A full NOTE for a live-note target. Accepted while loading or ready. */
+ acceptNote(note: NotebookNoteInput): boolean;
+ /** A full NOTE_REVISION for a revision target. A response without a note is `not-found`. */
+ acceptRevision(revision: NotebookRevisionInput): boolean;
+ /** Ends a load that has not completed. A failure ends this request; activate again to retry. */
+ fail(reason: NotebookReadFailure): boolean;
+ /** Starts a permissions read. A later call replaces an earlier one. */
+ readAcl(): NotebookAclRequest;
+}
+
+export interface NotebookReadStore {
+ /** Returns the same reference until the read state changes. */
+ getSnapshot(): NotebookReadSnapshot;
+ /** Listeners run in subscription order, once per snapshot change. */
+ subscribe(listener: () => void): () => void;
+ activate(target: NotebookReadTarget): NotebookReadRequest;
+ deactivate(): void;
+ /** Publishes a final `disposed` snapshot, then drops every listener. */
+ dispose(): void;
+}
+
+// Read state is built from wire data: null, booleans, numbers, strings, arrays and plain objects.
+const freezeCopy = (value: unknown): unknown => {
+ if (Array.isArray(value)) {
+ return Object.freeze(value.map(freezeCopy));
+ }
+ if (value !== null && typeof value === 'object') {
+ const copy: { [key: string]: unknown } = {};
+ for (const key of Object.keys(value)) {
+ copy[key] = freezeCopy((value as { [key: string]: unknown })[key]);
+ }
+ return Object.freeze(copy);
+ }
+ return value;
+};
+
+const sameValue = (a: unknown, b: unknown): boolean => {
+ if (a === b) {
+ return true;
+ }
+ if (Array.isArray(a) && Array.isArray(b)) {
+ return a.length === b.length && a.every((item, index) => sameValue(item, b[index]));
+ }
+ if (a !== null && b !== null && typeof a === 'object' && typeof b === 'object') {
+ if (Array.isArray(a) || Array.isArray(b)) {
+ return false;
+ }
+ const aKeys = Object.keys(a);
+ const bKeys = Object.keys(b);
+ return (
+ aKeys.length === bKeys.length &&
+ aKeys.every(
+ key =>
+ Object.prototype.hasOwnProperty.call(b, key) &&
+ sameValue((a as { [key: string]: unknown })[key], (b as { [key: string]: unknown })[key])
+ )
+ );
+ }
+ return false;
+};
+
+/** Keeps the previous object when the next one has the same content. */
+const reuse = <T>(previous: T | undefined, next: T): T =>
+ previous !== undefined && sameValue(previous, next) ? previous : next;
+
+const emptyAcl = (status: NotebookAclStatus): NotebookReadAcl =>
+ freezeCopy({ status, owners: [], readers: [], runners: [], writers: [] }) as NotebookReadAcl;
+
+const unknownAcl = emptyAcl('unknown');
+const loadingAcl = emptyAcl('loading');
+
+const buildParagraph = (input: NotebookParagraphInput): NotebookReadParagraph => {
+ const chartConfigs: { [resultIndex: string]: unknown } = {};
+ const configResults = input.config?.results ?? {};
+ for (const resultIndex of Object.keys(configResults)) {
+ const graph = configResults[resultIndex]?.graph;
+ if (graph !== undefined) {
+ chartConfigs[resultIndex] = graph;
+ }
+ }
+ return freezeCopy({
+ id: input.id,
+ title: input.title ?? null,
+ text: input.text ?? '',
+ status: input.status,
+ resultCode: input.results?.code ?? null,
+ resultMessages: (input.results?.msg ?? []).map(message => ({ type: message.type, data: message.data })),
+ chartConfigs
+ }) as NotebookReadParagraph;
+};
+
+const buildNote = (input: NotebookNoteInput, previous: NotebookReadNote | null): NotebookReadNote => {
+ const paragraphsById: { [paragraphId: string]: NotebookReadParagraph } = {};
+ for (const paragraphInput of input.paragraphs) {
+ const previousParagraph =
+ previous && Object.prototype.hasOwnProperty.call(previous.paragraphsById, paragraphInput.id)
+ ? previous.paragraphsById[paragraphInput.id]
+ : undefined;
+ paragraphsById[paragraphInput.id] = reuse(previousParagraph, buildParagraph(paragraphInput));
+ }
+ const next: NotebookReadNote = Object.freeze({
+ id: input.id,
+ name: input.name,
+ path: input.path ?? null,
+ paragraphIds: reuse(previous?.paragraphIds, Object.freeze(input.paragraphs.map(paragraph => paragraph.id))),
+ paragraphsById: Object.freeze(paragraphsById)
+ });
+ return reuse(previous ?? undefined, next);
+};
+
+const makeSnapshot = (
+ target: NotebookReadTarget | null,
+ status: NotebookReadStatus,
+ note: NotebookReadNote | null,
+ acl: NotebookReadAcl
+): NotebookReadSnapshot =>
+ Object.freeze({
+ noteId: target?.noteId ?? '',
+ revisionId: target?.kind === 'revision' ? target.revisionId : null,
+ target,
+ status,
+ note,
+ acl
+ });
+
+const idleSnapshot = makeSnapshot(null, 'idle', null, unknownAcl);
+const disposedSnapshot = makeSnapshot(null, 'disposed', null, unknownAcl);
+
+const staleAclRequest: NotebookAclRequest = Object.freeze({
+ isCurrent: () => false,
+ accept: () => false,
+ fail: () => false
+});
+
+export const createNotebookReadStore = (): NotebookReadStore => {
+ let snapshot = idleSnapshot;
+ // One entry per subscription, so subscribing the same function twice needs two unsubscribes.
+ let subscriptions: Array<{ listener: () => void }> = [];
+ // Identity tokens for the current activation and permissions read. A handle compares its own
+ // token with these, so a replaced handle stays rejected even when the same target returns.
+ let activeRequest: object | null = null;
+ let activeAclRequest: object | null = null;
+ let disposed = false;
+
+ const publish = (next: NotebookReadSnapshot): void => {
+ if (next === snapshot) {
+ return;
+ }
+ snapshot = next;
+ // subscribe and unsubscribe replace the array, so changes made while notifying take effect
+ // from the next change.
+ for (const subscription of subscriptions) {
+ subscription.listener();
+ }
+ };
+
+ // Changes the read state of the current target; activation publishes a new target directly.
+ const update = (changes: Partial<Pick<NotebookReadSnapshot, 'status' | 'note' | 'acl'>>): void => {
+ const status = changes.status ?? snapshot.status;
+ const note = 'note' in changes ? (changes.note ?? null) : snapshot.note;
+ const acl = changes.acl ?? snapshot.acl;
+ if (status === snapshot.status && note === snapshot.note && acl === snapshot.acl) {
+ return;
+ }
+ publish(makeSnapshot(snapshot.target, status, note, acl));
+ };
+
+ const activate = (requestedTarget: NotebookReadTarget): NotebookReadRequest => {
+ if (disposed) {
+ throw new Error('Cannot activate a disposed notebook read store');
+ }
+ const token = {};
+ activeRequest = token;
+ activeAclRequest = null;
+ const target = freezeCopy(requestedTarget) as NotebookReadTarget;
+ publish(makeSnapshot(target, 'loading', null, unknownAcl));
+
+ const isCurrent = (): boolean => activeRequest === token;
+ const canLoad = (): boolean => isCurrent() && (snapshot.status === 'loading' || snapshot.status === 'ready');
+
+ // A permissions re-read passes through `loading`; reuse the last lists read in this activation.
+ let lastReadyAcl: NotebookReadAcl | undefined;
+
+ const acceptFullNote = (input: NotebookNoteInput): void => {
+ const previous = snapshot.status === 'ready' ? snapshot.note : null;
+ update({ status: 'ready', note: buildNote(input, previous) });
+ };
+
+ const readAcl = (): NotebookAclRequest => {
+ if (!isCurrent()) {
+ return staleAclRequest;
+ }
+ const aclToken = {};
+ activeAclRequest = aclToken;
+ update({ acl: loadingAcl });
+ const isAclCurrent = (): boolean => isCurrent() && activeAclRequest === aclToken;
+ return Object.freeze({
+ isCurrent: isAclCurrent,
+ accept: (input: NotebookAclInput): boolean => {
+ if (!isAclCurrent()) {
+ return false;
+ }
+ const acl = freezeCopy({
+ status: 'ready',
+ owners: input.owners,
+ readers: input.readers,
+ runners: input.runners,
+ writers: input.writers
+ }) as NotebookReadAcl;
+ lastReadyAcl = reuse(lastReadyAcl, acl);
+ update({ acl: lastReadyAcl });
+ return true;
+ },
+ fail: (reason: 'access-denied' | 'failed'): boolean => {
+ if (!isAclCurrent()) {
+ return false;
+ }
+ update({ acl: emptyAcl(reason) });
+ return true;
+ }
+ });
+ };
+
+ return Object.freeze({
+ target,
+ isCurrent,
+ acceptNote: (input: NotebookNoteInput): boolean => {
+ if (!canLoad() || target.kind !== 'note' || input.id !== target.noteId) {
+ return false;
+ }
+ acceptFullNote(input);
+ return true;
+ },
+ acceptRevision: (input: NotebookRevisionInput): boolean => {
+ if (
+ !canLoad() ||
+ target.kind !== 'revision' ||
+ input.noteId !== target.noteId ||
+ input.revisionId !== target.revisionId
+ ) {
+ return false;
+ }
+ if (input.note === undefined || input.note === null) {
+ if (snapshot.status !== 'loading') {
+ return false;
+ }
+ update({ status: 'not-found', note: null });
Review Comment:
A `NOTE_REVISION` response without a note does not necessarily mean `not-found`: `Notebook#getNoteByRevision` also returns `null` when the default repository does not support revisions. Since the cause is unknown here, could we report `failed` instead and update the corresponding PR description?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-6736] Implement Shared Notebook Core read state and selectors [zeppelin]
Posted by "voidmatcha (via GitHub)" <gi...@apache.org>.
voidmatcha commented on PR #5559:
URL: https://github.com/apache/zeppelin/pull/5559#issuecomment-6076790525
The Playwright tests did not run because both E2E jobs stopped at the Prettier check. The reported file was added in #5540 and is outside this PR diff. Could it be formatted in a small follow-up, then the E2E jobs rerun?
```bash
cd zeppelin-web-angular
npx prettier --write e2e/core-contract/lifecycle/core/runtime/host-remote-contract.ts
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org