Copilot commented on code in PR #7754: URL: https://github.com/apache/texera/pull/7754#discussion_r3802048721
########## sql/updates/38.sql: ########## @@ -0,0 +1,35 @@ +/* + * 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; Review Comment: `\\c` is a psql meta-command and will fail when executed via Liquibase over JDBC. Also, explicit `BEGIN;`/`COMMIT;` inside a Liquibase `sqlFile` commonly conflicts with Liquibase-managed transactions (can cause 'cannot be used in a transaction block' / nested transaction issues depending on configuration). Prefer removing `\\c` and the explicit transaction statements, and rely on the Liquibase connection/database plus the changeSet transaction. ########## amber/src/main/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResource.scala: ########## @@ -131,20 +132,34 @@ class WarehouseResource(client: LakekeeperClient, enabled: Boolean) extends Lazy throw new WebApplicationException(s"a warehouse named '$name' already exists", 409) } - val warehouseName = s"user-$uid-$name" + // The catalog name is derived from the row's own id, never from `name`: that one + // string is also the REST catalog prefix, the S3 key prefix and a component of every + // result URI an execution wrote, so deriving it from a user-facing name would freeze + // that name forever (#7753). Take the id from the sequence up front so the creation + // order below is unchanged -- Lakekeeper first, row after, with the compensating + // delete. The sequence is resolved from the catalog rather than named literally, + // because its generated name is not a stable contract. + val whid: Integer = context.fetchValue( + DSL.field( + "nextval(pg_get_serial_sequence('texera_db.user_warehouse','whid'))", + classOf[Integer] Review Comment: The schema name `texera_db` is hardcoded in the `pg_get_serial_sequence` call. This makes the code brittle if the schema changes (or differs across environments/tests). Prefer deriving the qualified table name from jOOQ metadata (schema/table names) or omitting the schema qualification and relying on the connection's `search_path` (while keeping the sequence-name indirection you want). ########## sql/updates/38.sql: ########## @@ -0,0 +1,35 @@ +/* + * 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; + +-- The Lakekeeper catalog name is no longer derived from the user-facing name +-- (#7753), and the table already has its own `name` column -- so the old +-- `warehouse_name` was ambiguous. Rename it to sit beside its sibling +-- `lakekeeper_warehouse_id`. The table is empty in every deployment (the +-- warehouse feature flag is off everywhere), so this carries no data. +ALTER TABLE user_warehouse + RENAME COLUMN warehouse_name TO lakekeeper_warehouse_name; + +COMMIT; Review Comment: `\\c` is a psql meta-command and will fail when executed via Liquibase over JDBC. Also, explicit `BEGIN;`/`COMMIT;` inside a Liquibase `sqlFile` commonly conflicts with Liquibase-managed transactions (can cause 'cannot be used in a transaction block' / nested transaction issues depending on configuration). Prefer removing `\\c` and the explicit transaction statements, and rely on the Liquibase connection/database plus the changeSet transaction. ########## amber/src/test/scala/org/apache/texera/web/resource/dashboard/user/warehouse/WarehouseResourceSpec.scala: ########## @@ -164,7 +180,17 @@ class WarehouseResourceSpec val squatter = getDSLContext.newRecord(USER_WAREHOUSE) squatter.setUid(otherUser.getUid) squatter.setName("unrelated") - squatter.setWarehouseName(s"user-${sessionUser.getUid}-boom") + // The catalog name now comes from the sequence, so claim the id the next create + // will draw: take one number for the squatter itself (set explicitly, so storing it + // consumes nothing further) and squat on the one after it. + val takenWhid = getDSLContext.fetchValue( + DSL.field( + "nextval(pg_get_serial_sequence('texera_db.user_warehouse','whid'))", + classOf[Integer] + ) + ) + squatter.setWhid(takenWhid) + squatter.setLakekeeperWarehouseName(s"user-${sessionUser.getUid}-${takenWhid + 1}") Review Comment: These compensation tests depend on global sequence state and assume the next create will consume exactly `takenWhid + 1`. This can become flaky if tests run in parallel or if any other code consumes the same sequence during the test. To make this deterministic, serialize access around the sequence (e.g., acquire a Postgres advisory lock / lock the relevant table within the test transaction) or mark the suite to run without parallel execution so no other test can advance the sequence between `nextval` and `resource.create`. ########## common/dao/src/test/scala/org/apache/texera/dao/UserWarehouseSpec.scala: ########## @@ -78,7 +78,7 @@ class UserWarehouseSpec extends AnyFlatSpec with Matchers with BeforeAndAfterAll .where(USER_WAREHOUSE.UID.eq(uid)) .fetchOne() row.getName shouldBe "mybucket" - row.getWarehouseName shouldBe s"user-$uid-mybucket" + row.getLakekeeperWarehouseName shouldBe s"user-$uid-mybucket" Review Comment: This test still asserts that the Lakekeeper warehouse/catalog name is derived from the display name (`mybucket`). That contradicts the new behavior introduced in this PR (`user-<uid>-<whid>`), so the test is now encoding the old contract. Update the expectation to assert the whid-derived format (e.g., using the fetched row's `whid`) so the DAO layer doesn't regress back to a display-name-derived identifier. -- 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]
