mengw15 commented on code in PR #7851:
URL: https://github.com/apache/texera/pull/7851#discussion_r3875574628
##########
amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/workflow/WorkflowResource.scala:
##########
@@ -93,6 +93,12 @@ object WorkflowResource {
}
private def insertWorkflow(workflow: Workflow, user: User): Unit = {
+ // A workflow is born with nothing pinned. The endpoint takes a whole
Workflow, so without this
+ // a request body could seed a published copy of its own choosing.
+ workflow.setPublishedVersionId(null)
+ workflow.setPublishedContent(null)
Review Comment:
This closes the insert path, but `persistWorkflow` also takes a whole
`Workflow` from the request body and hands it to `workflowDao.update(workflow)`
in both of its update branches (lines 465 and 482), which writes every column.
So the columns are still client-writable on an existing workflow — the same
hole Copilot flagged, one endpoint over, and `/workflow/persist` is the
autosave path.
It is reachable in this PR alone: persist `{isPublic: true,
publishedContent: "..."}` on your own workflow (the CHECK passes while it is
public), then `PUT /workflow/private/{wid}` refetches that row, flips
`is_public` and updates — which now violates `workflow_pin_requires_public` and
500s. The workflow cannot be made private again.
Relatedly, `texera_ddl.sql` still says these columns "are only ever written
by WorkflowPublishService, which clears them on unpublish" — that class does
not exist yet, and as of this PR two other paths write them, one of them
client-controlled.
##########
sql/updates/42.sql:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+
+\c texera_db
+
+SET search_path TO texera_db;
+
+BEGIN;
+
+-- Version pinning: a public workflow follows the author's latest until they
pin the version they
+-- have now, after which the public keeps seeing that frozen copy. is_public
stays the on/off switch;
+-- published_content is the pin, NULL while following. Materialized rather
than replayed from
+-- workflow_version, whose rows are reverse deltas that no fulltext index can
cover.
+ALTER TABLE workflow
+ -- The version row holding the pinned copy. Its delta is the identity
patch, so replaying it
+ -- returns exactly what is on public show; the revision panel marks that
row, which is how the
+ -- author restores the public version into their editor.
+ ADD COLUMN IF NOT EXISTS published_version_id INT,
+ ADD COLUMN IF NOT EXISTS published_content TEXT,
+ ADD COLUMN IF NOT EXISTS published_name VARCHAR(128),
+ ADD COLUMN IF NOT EXISTS published_description TEXT;
+
+-- No backfill. Every workflow that is public today has no pin, which is the
following state, which is
+-- exactly what it does today: deploying this migration changes nothing anyone
can see. Pinning is
+-- something an author opts into afterwards.
+
+-- A pin only means something while the workflow is public, so a private
workflow must not carry one.
+-- Making that unrepresentable is cheaper than catching it: unpublishing
clears the pin, and no other
+-- path writes these columns.
+-- Dropped and re-added rather than guarded on a name lookup: constraint names
are unique per table,
+-- not per database, so a same-named constraint on any other table would make
the guard skip this one
+-- and leave the workflow table without it. Mirrors texera_ddl.sql.
+ALTER TABLE workflow
+ DROP CONSTRAINT IF EXISTS workflow_published_consistent;
+ALTER TABLE workflow
+ DROP CONSTRAINT IF EXISTS workflow_pin_requires_public;
+ALTER TABLE workflow
+ ADD CONSTRAINT workflow_pin_requires_public
+ CHECK (published_content IS NULL OR is_public);
Review Comment:
The constraint covers `published_content` only — `published_name`,
`published_description` and `published_version_id` can each sit non-NULL on a
private row, and a path that clears three of the four leaves the fourth behind.
Writing the whole group covers what the description claims:
```sql
CHECK (
(published_content IS NULL AND published_name IS NULL
AND published_description IS NULL AND published_version_id IS NULL)
OR (is_public AND published_content IS NOT NULL AND published_name IS NOT
NULL)
)
```
##########
sql/updates/42.sql:
##########
@@ -0,0 +1,92 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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.
+ */
+
+\c texera_db
+
+SET search_path TO texera_db;
+
+BEGIN;
+
+-- Version pinning: a public workflow follows the author's latest until they
pin the version they
+-- have now, after which the public keeps seeing that frozen copy. is_public
stays the on/off switch;
+-- published_content is the pin, NULL while following. Materialized rather
than replayed from
+-- workflow_version, whose rows are reverse deltas that no fulltext index can
cover.
+ALTER TABLE workflow
+ -- The version row holding the pinned copy. Its delta is the identity
patch, so replaying it
+ -- returns exactly what is on public show; the revision panel marks that
row, which is how the
+ -- author restores the public version into their editor.
+ ADD COLUMN IF NOT EXISTS published_version_id INT,
+ ADD COLUMN IF NOT EXISTS published_content TEXT,
+ ADD COLUMN IF NOT EXISTS published_name VARCHAR(128),
+ ADD COLUMN IF NOT EXISTS published_description TEXT;
+
+-- No backfill. Every workflow that is public today has no pin, which is the
following state, which is
+-- exactly what it does today: deploying this migration changes nothing anyone
can see. Pinning is
+-- something an author opts into afterwards.
+
+-- A pin only means something while the workflow is public, so a private
workflow must not carry one.
+-- Making that unrepresentable is cheaper than catching it: unpublishing
clears the pin, and no other
+-- path writes these columns.
+-- Dropped and re-added rather than guarded on a name lookup: constraint names
are unique per table,
+-- not per database, so a same-named constraint on any other table would make
the guard skip this one
+-- and leave the workflow table without it. Mirrors texera_ddl.sql.
+ALTER TABLE workflow
+ DROP CONSTRAINT IF EXISTS workflow_published_consistent;
Review Comment:
`workflow_published_consistent` is not created anywhere in the tree, so this
drop is a no-op — a fresh apply logs `constraint
"workflow_published_consistent" ... does not exist, skipping`. Same line in
`texera_ddl.sql`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
