Copilot commented on code in PR #3710: URL: https://github.com/apache/texera/pull/3710#discussion_r2314597115
########## core/scripts/sql/updates/13.sql: ########## @@ -0,0 +1,84 @@ +/* + * 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; + +-- Rename the old table to migrate later +ALTER TABLE user_activity RENAME TO user_action_old; + +-- Validate existing values for "activate" +DO $do$ + BEGIN + IF EXISTS ( + SELECT 1 FROM user_action_old + WHERE lower(activate) NOT IN ('like','unlike','clone','view') + ) THEN + RAISE EXCEPTION 'Error.'; Review Comment: The error message 'Error.' is not descriptive. It should provide specific information about what validation failed, such as 'Invalid action value found in user_activity table. Only like, unlike, clone, and view are allowed.' ```suggestion RAISE EXCEPTION 'Invalid action value found in user_action_old.activate. Only like, unlike, clone, and view are allowed.'; ``` ########## core/scripts/sql/updates/13.sql: ########## @@ -0,0 +1,84 @@ +/* + * 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; + +-- Rename the old table to migrate later +ALTER TABLE user_activity RENAME TO user_action_old; + +-- Validate existing values for "activate" +DO $do$ + BEGIN + IF EXISTS ( + SELECT 1 FROM user_action_old + WHERE lower(activate) NOT IN ('like','unlike','clone','view') + ) THEN + RAISE EXCEPTION 'Error.'; + END IF; + END +$do$; + +-- Create enum type +DO $do$ + BEGIN + IF NOT EXISTS ( + SELECT 1 + FROM pg_type t + JOIN pg_namespace n ON n.oid = t.typnamespace + WHERE t.typname = 'action_enum' AND n.nspname = 'texera_db' + ) THEN + EXECUTE 'CREATE TYPE texera_db.action_enum AS ENUM (''like'',''unlike'',''clone'',''view'')'; + END IF; + END +$do$; + +-- Create the new table +CREATE TABLE user_action ( + user_action_id BIGSERIAL PRIMARY KEY, + uid INTEGER, + ip VARCHAR(15), + "time" TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + resource_type VARCHAR(15) NOT NULL, + resource_id INTEGER NOT NULL, + action texera_db.action_enum NOT NULL +); + +-- Copy data +INSERT INTO user_action (uid, ip, "time", resource_type, resource_id, action) +SELECT Review Comment: The magic number `0` should be explained with a comment or replaced with a named constant. It's unclear why uid = 0 is treated as a special case that should be converted to NULL. ```suggestion -- In the legacy schema, ua.uid = 0 indicates an anonymous or missing user. We convert this to NULL in the new schema. INSERT INTO user_action (uid, ip, "time", resource_type, resource_id, action) ``` ########## core/amber/src/main/scala/edu/uci/ics/texera/web/resource/dashboard/hub/HubResource.scala: ########## @@ -148,6 +149,12 @@ object HubResource { buffer.toList.asJava } + def toActionEnum(a: ActionType): ActionEnum = { + ActionEnum.values() + .find(_.getLiteral.equalsIgnoreCase(a.value)) + .getOrElse(throw new IllegalArgumentException(s"Unsupported action: ${a.value}")) + } Review Comment: The `ActionEnum.values()` call creates a new array on each invocation. Consider caching the enum values in a companion object or using a more efficient lookup method to avoid repeated array allocation. -- 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]
