feat(custom-fields): カスタムフィールド機能実装#108
Open
yupix wants to merge 4 commits into
Open
Conversation
Add project_custom_fields and task_custom_field_values migrations for per-project task attribute definitions and values. Assisted-by: multi-agent-shogun-aki-tweak
Implement CRUD API for project-scoped custom field definitions with validation helpers and OpenAPI documentation. Assisted-by: multi-agent-shogun-aki-tweak
Expose custom_field_values on task create/get/update and enforce required field validation when transitioning to done status. Assisted-by: multi-agent-shogun-aki-tweak
Cover field definition CRUD, select validation, and task value persistence via the custom fields integration suite. Assisted-by: multi-agent-shogun-aki-tweak
Walkthroughこのプルリクエストは、プロジェクト内でタスクに任意のカスタムフィールドを定義・管理するための機能を追加します。データベーススキーマからAPIまで完全なカスタムフィールド機能を実装しており、テキスト・数値・選択肢・日付・URL・チェックボックス型をサポートしています。 Changesカスタムフィールド機能の実装
Sequence DiagramssequenceDiagram
participant Client
participant Task Handler
participant Custom Fields Utils
participant Database
Client->>Task Handler: POST /tasks with custom_field_values
Task Handler->>Custom Fields Utils: upsert_task_custom_field_values
Custom Fields Utils->>Custom Fields Utils: validate each value by field type
Custom Fields Utils->>Database: insert/update/delete values
Database-->>Custom Fields Utils: confirm
Custom Fields Utils-->>Task Handler: OK
Task Handler->>Custom Fields Utils: ensure_required_custom_fields
Custom Fields Utils->>Database: load required fields and existing values
Database-->>Custom Fields Utils: field list
Custom Fields Utils->>Custom Fields Utils: check all required fields have non-empty values
Custom Fields Utils-->>Task Handler: OK or BadRequest
Task Handler->>Database: insert task
Database-->>Task Handler: task
Task Handler->>Custom Fields Utils: load_task_custom_field_values
Custom Fields Utils->>Database: fetch field definitions and values
Database-->>Custom Fields Utils: values with display_value
Custom Fields Utils-->>Task Handler: TaskCustomFieldValueResponse array
Task Handler-->>Client: 201 Created with TaskDetailResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/migration/src/m20260609_create_custom_fields.rs`:
- Line 18: migration の SQL で project_custom_fields テーブルの position 列に DEFAULT
値が無いため挿入時に明示指定が必要になっています。m20260609_create_custom_fields.rs の CREATE TABLE 定義内で
position SMALLINT NOT NULL に対してデフォルトを追加するか（例: DEFAULT COALESCE((SELECT
MAX(position)+1 FROM project_custom_fields WHERE project_id = NEW.project_id),
0) を設定するトリガーを作る）、もしくは position に対するシーケンスを導入して自動採番するように変更して、ハンドラ側で毎回
MAX(position)+1 を計算しなくて済むようにしてください（対象シンボル: project_custom_fields テーブル, position
カラム, m20260609_create_custom_fields.rs）。
- Line 19: Add an updated_at TIMESTAMPTZ column with a default and migration
handling so updates are tracked: in the migration that defines the table
(m20260609_create_custom_fields.rs) add "updated_at TIMESTAMPTZ NOT NULL DEFAULT
now()," alongside created_at, and ensure the migration's rollback/drop and any
INSERT/UPDATE logic or triggers (if you use automatic update-on-change) are
updated to set/maintain updated_at accordingly.
- Around line 11-21: Add a composite index on (project_id, position) to the
project_custom_fields creation to optimize queries that filter by project_id and
order by position (referenced in utils/custom_fields.rs
load_task_custom_field_values around Line 87); update the migration
m20260609_create_custom_fields.rs to create an index for (project_id, position)
after the table DDL (or include it inline) so queries that filter on project_id
and sort by position can use the index for better performance.
- Line 15: 現在のマイグレーションの field_type カラムが VARCHAR NOT NULL
のままで任意の文字列が入ってしまうため、テーブル定義を更新して許容値をスキーマレベルで制限してください（m20260609_create_custom_fields.rs
の該当 CREATE TABLE/alter 定義内の field_type を修正）。具体的には、アプリケーションで想定する値 ("Text",
"Number", "Select", "Date", "Url", "Checkbox") のみを許可するように PostgreSQL の CHECK
制約を追加するか、あるいは ENUM 型を作成して field_type に適用してください（参照箇所: マイグレーション内の field_type 列定義）。
In `@apps/backend/src/handlers/custom_fields.rs`:
- Around line 105-107: The select options validator validate_select_options
currently only checks for presence of label/value keys and allows non-string or
empty values; update validate_select_options (in utils/custom_fields.rs) to
assert that each option's "label" and "value" are both strings and non-empty
(trimmed) and return an error if not, so callers like the branch handling
CustomFieldType::Select in handlers/custom_fields.rs and the other
options-present branch reject invalid definitions; keep the same error type/flow
so existing callers still use the ? propagation.
- Around line 126-127: The update endpoint in
apps/backend/src/handlers/custom_fields.rs currently registers the HTTP method
as `put` for the route with `path = "/{field_id}"`; change that token to `patch`
so the handler matches the public contract `PATCH
/projects/{id}/custom-fields/{field_id}`—update the route attribute (the line
containing `put, path = "/{field_id}"`) to `patch, path = "/{field_id}"` so the
handler registration (and any related handler function for updating a custom
field) uses PATCH.
In `@apps/backend/src/utils/custom_fields.rs`:
- Around line 97-126: The upsert_task_custom_field_values function currently
issues unconditional DELETEs per-empty value, lacks transaction boundaries and
is vulnerable to race conditions; fix by wrapping the whole operation in a DB
transaction (begin a transaction via the ConnectionTrait implementation) and
replace the SELECT→UPDATE/INSERT and unconditional DELETE logic on
task_custom_field_values::Entity / task_custom_field_values::ActiveModel with a
single upsert (INSERT ... ON CONFLICT (task_id, field_id) DO UPDATE SET value =
EXCLUDED.value) for non-empty values and a conditional DELETE only when a
matching row exists (or use a DELETE that returns affected rows) for empty
values, keeping validate_custom_field_value calls intact and referring to
input.field_id and task_id to form the conflict key so concurrent requests are
safe and partial updates are rolled back on error.
- Line 11: DATE_REGEX only validates the YYYY-MM-DD format and allows invalid
dates like 2024-02-30; update the validation logic that uses DATE_REGEX (or
replace its usage) to parse the string with chrono::NaiveDate::parse_from_str
(e.g., try NaiveDate::parse_from_str(date_str, "%Y-%m-%d")) and treat parse
errors as invalid dates so only truly valid calendar dates are accepted; keep or
use DATE_REGEX for a fast pre-check if desired but rely on
NaiveDate::parse_from_str for definitive validation.
- Around line 46-55: validate_select_options currently only checks presence of
"label" and "value" keys but allows empty strings, non-string values, and
duplicates; update validate_select_options to (1) ensure each item's "label" and
"value" are strings and not empty, returning AppError::BadRequest on failure,
(2) validate that "value" specifically is a string type (using Value::as_str)
and non-empty, and (3) detect duplicate "value" entries by collecting values
into a HashSet (or similar) and returning BadRequest if a duplicate is found;
use the existing options/as_array/as_object parsing flow in
validate_select_options to locate where to add these checks.
In `@apps/backend/tests/custom_fields_integration.rs`:
- Line 59: The test only asserts that custom_field_values is an array, which
lets an empty array pass; instead, parse the response JSON once (replace the
inline get.json::<Value>().await.expect("json") call with a let body =
get.json::<Value>().await.expect("json")) and assert that
body["custom_field_values"].as_array().map(|a| a.len()) == Some(2), then verify
each item's "field_id" and "value" match the expected pairs (compare strings/ids
for the two entries), e.g. iterate
body["custom_field_values"].as_array().unwrap() and assert field_id and value
equality for each expected record to ensure both count and content are
validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b7a60099-c5dc-4f56-ab67-36c9d9df4841
📒 Files selected for processing (13)
apps/backend/migration/src/lib.rsapps/backend/migration/src/m20260609_create_custom_fields.rsapps/backend/src/entities/mod.rsapps/backend/src/entities/project_custom_fields.rsapps/backend/src/entities/task_custom_field_values.rsapps/backend/src/handlers/custom_fields.rsapps/backend/src/handlers/mod.rsapps/backend/src/handlers/tasks.rsapps/backend/src/openapi/mod.rsapps/backend/src/routes/tenants.rsapps/backend/src/utils/custom_fields.rsapps/backend/src/utils/mod.rsapps/backend/tests/custom_fields_integration.rs
Comment on lines
+11
to
+21
| CREATE TABLE IF NOT EXISTS project_custom_fields ( | ||
| id UUID PRIMARY KEY, | ||
| project_id UUID NOT NULL REFERENCES projects(id) ON DELETE CASCADE, | ||
| name VARCHAR(100) NOT NULL, | ||
| field_type VARCHAR NOT NULL, | ||
| options JSONB, | ||
| is_required BOOLEAN NOT NULL DEFAULT false, | ||
| position SMALLINT NOT NULL, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), | ||
| UNIQUE (project_id, name) | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
position でのソートクエリ用の複合インデックスの追加を検討してください。
load_task_custom_field_values では project_id でフィルタし position でソートするクエリが実行されます（utils/custom_fields.rs Line 87）。(project_id, position) の複合インデックスを追加することで、大量のカスタムフィールドが存在する場合のパフォーマンスが向上します。
📊 インデックス追加案
created_at TIMESTAMPTZ NOT NULL DEFAULT now(),
UNIQUE (project_id, name)
);
+ CREATE INDEX IF NOT EXISTS idx_project_custom_fields_project_position
+ ON project_custom_fields(project_id, position);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| CREATE TABLE IF NOT EXISTS project_custom_fields ( | |
| id UUID PRIMARY KEY, | |
| project_id UUID NOT NULL REFERENCES projects(id) ON DELETE CASCADE, | |
| name VARCHAR(100) NOT NULL, | |
| field_type VARCHAR NOT NULL, | |
| options JSONB, | |
| is_required BOOLEAN NOT NULL DEFAULT false, | |
| position SMALLINT NOT NULL, | |
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), | |
| UNIQUE (project_id, name) | |
| ); | |
| CREATE TABLE IF NOT EXISTS project_custom_fields ( | |
| id UUID PRIMARY KEY, | |
| project_id UUID NOT NULL REFERENCES projects(id) ON DELETE CASCADE, | |
| name VARCHAR(100) NOT NULL, | |
| field_type VARCHAR NOT NULL, | |
| options JSONB, | |
| is_required BOOLEAN NOT NULL DEFAULT false, | |
| position SMALLINT NOT NULL, | |
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), | |
| UNIQUE (project_id, name) | |
| ); | |
| CREATE INDEX IF NOT EXISTS idx_project_custom_fields_project_position | |
| ON project_custom_fields(project_id, position); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/migration/src/m20260609_create_custom_fields.rs` around lines 11
- 21, Add a composite index on (project_id, position) to the
project_custom_fields creation to optimize queries that filter by project_id and
order by position (referenced in utils/custom_fields.rs
load_task_custom_field_values around Line 87); update the migration
m20260609_create_custom_fields.rs to create an index for (project_id, position)
after the table DDL (or include it inline) so queries that filter on project_id
and sort by position can use the index for better performance.
| id UUID PRIMARY KEY, | ||
| project_id UUID NOT NULL REFERENCES projects(id) ON DELETE CASCADE, | ||
| name VARCHAR(100) NOT NULL, | ||
| field_type VARCHAR NOT NULL, |
There was a problem hiding this comment.
field_type に制約がなく、データ整合性リスクがあります。
field_type が VARCHAR NOT NULL のみで定義されており、アプリケーションコードで想定される値（Text, Number, Select, Date, Url, Checkbox）以外の文字列も DB に保存できてしまいます。PostgreSQL の CHECK 制約または ENUM 型を使用して、スキーマレベルで許可される値を制限することを検討してください。
🛡️ CHECK 制約を使った修正案
- field_type VARCHAR NOT NULL,
+ field_type VARCHAR NOT NULL CHECK (field_type IN ('Text', 'Number', 'Select', 'Date', 'Url', 'Checkbox')),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| field_type VARCHAR NOT NULL, | |
| field_type VARCHAR NOT NULL CHECK (field_type IN ('Text', 'Number', 'Select', 'Date', 'Url', 'Checkbox')), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/migration/src/m20260609_create_custom_fields.rs` at line 15,
現在のマイグレーションの field_type カラムが VARCHAR NOT NULL
のままで任意の文字列が入ってしまうため、テーブル定義を更新して許容値をスキーマレベルで制限してください（m20260609_create_custom_fields.rs
の該当 CREATE TABLE/alter 定義内の field_type を修正）。具体的には、アプリケーションで想定する値 ("Text",
"Number", "Select", "Date", "Url", "Checkbox") のみを許可するように PostgreSQL の CHECK
制約を追加するか、あるいは ENUM 型を作成して field_type に適用してください（参照箇所: マイグレーション内の field_type 列定義）。
Source: Coding guidelines
| field_type VARCHAR NOT NULL, | ||
| options JSONB, | ||
| is_required BOOLEAN NOT NULL DEFAULT false, | ||
| position SMALLINT NOT NULL, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
position に DEFAULT 値がなく、挿入時に明示的な指定が必要です。
position フィールドに DEFAULT 値または自動採番の仕組みがないため、新規フィールド作成時に毎回 position を計算して指定する必要があります。ハンドラ側で MAX(position) + 1 を取得する実装になっていると思われますが、DEFAULT 値（例: COALESCE((SELECT MAX(position) + 1 FROM project_custom_fields WHERE project_id = NEW.project_id), 0)）をトリガーで設定するか、シーケンスを使うことで、挿入ロジックをシンプルにできます。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/migration/src/m20260609_create_custom_fields.rs` at line 18,
migration の SQL で project_custom_fields テーブルの position 列に DEFAULT
値が無いため挿入時に明示指定が必要になっています。m20260609_create_custom_fields.rs の CREATE TABLE 定義内で
position SMALLINT NOT NULL に対してデフォルトを追加するか（例: DEFAULT COALESCE((SELECT
MAX(position)+1 FROM project_custom_fields WHERE project_id = NEW.project_id),
0) を設定するトリガーを作る）、もしくは position に対するシーケンスを導入して自動採番するように変更して、ハンドラ側で毎回
MAX(position)+1 を計算しなくて済むようにしてください（対象シンボル: project_custom_fields テーブル, position
カラム, m20260609_create_custom_fields.rs）。
| options JSONB, | ||
| is_required BOOLEAN NOT NULL DEFAULT false, | ||
| position SMALLINT NOT NULL, | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT now(), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
updated_at カラムがないため、フィールド定義の更新履歴を追跡できません。
監査やデバッグのために、updated_at TIMESTAMPTZ カラムを追加することを検討してください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/migration/src/m20260609_create_custom_fields.rs` at line 19, Add
an updated_at TIMESTAMPTZ column with a default and migration handling so
updates are tracked: in the migration that defines the table
(m20260609_create_custom_fields.rs) add "updated_at TIMESTAMPTZ NOT NULL DEFAULT
now()," alongside created_at, and ensure the migration's rollback/drop and any
INSERT/UPDATE logic or triggers (if you use automatic update-on-change) are
updated to set/maintain updated_at accordingly.
Comment on lines
+105
to
+107
| if payload.field_type == project_custom_fields::CustomFieldType::Select { | ||
| validate_select_options(&payload.options)?; | ||
| } else if payload.options.is_some() { |
There was a problem hiding this comment.
select の options 検証が弱く、無効な定義を保存できます。
Line 106/155 で呼んでいる validate_select_options（apps/backend/src/utils/custom_fields.rs Line 46-55）は label/value キーの存在しか見ていません。文字列でない値が通ると、後段の値検証（同ファイル Line 57-75 の as_str）で候補から落ち、作成済みフィールドが実質利用不能になります。label/value を文字列かつ非空まで検証してください。
🔧 修正案（utils 側）
pub fn validate_select_options(options: &Option<Value>) -> Result<(), AppError> {
let Some(options) = options else { return Err(AppError::BadRequest); };
let arr = options.as_array().ok_or(AppError::BadRequest)?;
if arr.is_empty() { return Err(AppError::BadRequest); }
for item in arr {
let obj = item.as_object().ok_or(AppError::BadRequest)?;
- if !obj.contains_key("label") || !obj.contains_key("value") { return Err(AppError::BadRequest); }
+ let label = obj.get("label").and_then(|v| v.as_str()).map(str::trim);
+ let value = obj.get("value").and_then(|v| v.as_str()).map(str::trim);
+ if label.map(|s| s.is_empty()).unwrap_or(true) || value.map(|s| s.is_empty()).unwrap_or(true) {
+ return Err(AppError::BadRequest);
+ }
}
Ok(())
}Also applies to: 153-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/handlers/custom_fields.rs` around lines 105 - 107, The
select options validator validate_select_options currently only checks for
presence of label/value keys and allows non-string or empty values; update
validate_select_options (in utils/custom_fields.rs) to assert that each option's
"label" and "value" are both strings and non-empty (trimmed) and return an error
if not, so callers like the branch handling CustomFieldType::Select in
handlers/custom_fields.rs and the other options-present branch reject invalid
definitions; keep the same error type/flow so existing callers still use the ?
propagation.
Comment on lines
+126
to
+127
| put, | ||
| path = "/{field_id}", |
There was a problem hiding this comment.
更新エンドポイントのHTTPメソッドが契約と不一致です。
Line 126 が put ですが、本PRの公開契約は PATCH /projects/{id}/custom-fields/{field_id} です。現状のままだとクライアント実装と OpenAPI の期待がずれます。patch に合わせてください。
🔧 修正案
- put,
+ patch,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| put, | |
| path = "/{field_id}", | |
| patch, | |
| path = "/{field_id}", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/handlers/custom_fields.rs` around lines 126 - 127, The
update endpoint in apps/backend/src/handlers/custom_fields.rs currently
registers the HTTP method as `put` for the route with `path = "/{field_id}"`;
change that token to `patch` so the handler matches the public contract `PATCH
/projects/{id}/custom-fields/{field_id}`—update the route attribute (the line
containing `put, path = "/{field_id}"`) to `patch, path = "/{field_id}"` so the
handler registration (and any related handler function for updating a custom
field) uses PATCH.
| use crate::entities::{project_custom_fields, task_custom_field_values}; | ||
| use crate::error::AppError; | ||
|
|
||
| static DATE_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"^\d{4}-\d{2}-\d{2}$").unwrap()); |
There was a problem hiding this comment.
日付検証が形式のみで、無効な日付を受け入れます。
DATE_REGEX は \d{4}-\d{2}-\d{2} 形式のみをチェックしており、2024-02-30 や 2024-13-01 のような無効な日付も通過してしまいます。chrono::NaiveDate::parse_from_str などを使用して、実際に有効な日付であることを確認することを推奨します。
📅 改善案
-static DATE_REGEX: LazyLock<Regex> = LazyLock::new(|| Regex::new(r"^\d{4}-\d{2}-\d{2}$").unwrap());
+use chrono::NaiveDate; project_custom_fields::CustomFieldType::Date => {
- if DATE_REGEX.is_match(value) { Ok(()) } else { Err(AppError::BadRequest) }
+ NaiveDate::parse_from_str(value, "%Y-%m-%d")
+ .map(|_| ())
+ .map_err(|_| AppError::BadRequest)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/utils/custom_fields.rs` at line 11, DATE_REGEX only
validates the YYYY-MM-DD format and allows invalid dates like 2024-02-30; update
the validation logic that uses DATE_REGEX (or replace its usage) to parse the
string with chrono::NaiveDate::parse_from_str (e.g., try
NaiveDate::parse_from_str(date_str, "%Y-%m-%d")) and treat parse errors as
invalid dates so only truly valid calendar dates are accepted; keep or use
DATE_REGEX for a fast pre-check if desired but rely on NaiveDate::parse_from_str
for definitive validation.
Comment on lines
+46
to
+55
| pub fn validate_select_options(options: &Option<Value>) -> Result<(), AppError> { | ||
| let Some(options) = options else { return Err(AppError::BadRequest); }; | ||
| let arr = options.as_array().ok_or(AppError::BadRequest)?; | ||
| if arr.is_empty() { return Err(AppError::BadRequest); } | ||
| for item in arr { | ||
| let obj = item.as_object().ok_or(AppError::BadRequest)?; | ||
| if !obj.contains_key("label") || !obj.contains_key("value") { return Err(AppError::BadRequest); } | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
validate_select_options の検証が不十分です。
現在の実装では以下の問題があります：
labelやvalueが空文字列でも通過してしまうvalueが文字列型であることを確認していない- 重複した
valueのチェックがない
選択肢の品質を保証するために、これらの検証を追加することを推奨します。
🔍 検証強化案
pub fn validate_select_options(options: &Option<Value>) -> Result<(), AppError> {
let Some(options) = options else { return Err(AppError::BadRequest); };
let arr = options.as_array().ok_or(AppError::BadRequest)?;
if arr.is_empty() { return Err(AppError::BadRequest); }
+ let mut seen_values = HashSet::new();
for item in arr {
let obj = item.as_object().ok_or(AppError::BadRequest)?;
- if !obj.contains_key("label") || !obj.contains_key("value") { return Err(AppError::BadRequest); }
+ let label = obj.get("label").and_then(|v| v.as_str()).ok_or(AppError::BadRequest)?;
+ let value = obj.get("value").and_then(|v| v.as_str()).ok_or(AppError::BadRequest)?;
+ if label.is_empty() || value.is_empty() { return Err(AppError::BadRequest); }
+ if !seen_values.insert(value) { return Err(AppError::BadRequest); }
}
Ok(())
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/utils/custom_fields.rs` around lines 46 - 55,
validate_select_options currently only checks presence of "label" and "value"
keys but allows empty strings, non-string values, and duplicates; update
validate_select_options to (1) ensure each item's "label" and "value" are
strings and not empty, returning AppError::BadRequest on failure, (2) validate
that "value" specifically is a string type (using Value::as_str) and non-empty,
and (3) detect duplicate "value" entries by collecting values into a HashSet (or
similar) and returning BadRequest if a duplicate is found; use the existing
options/as_array/as_object parsing flow in validate_select_options to locate
where to add these checks.
Comment on lines
+97
to
+126
| pub async fn upsert_task_custom_field_values<C: ConnectionTrait>(db: &C, project_id: Uuid, task_id: Uuid, inputs: &[CustomFieldValueInput]) -> Result<(), AppError> { | ||
| if inputs.is_empty() { return Ok(()); } | ||
| let field_ids: Vec<Uuid> = inputs.iter().map(|i| i.field_id).collect(); | ||
| let unique_ids: HashSet<Uuid> = field_ids.iter().copied().collect(); | ||
| if unique_ids.len() != field_ids.len() { return Err(AppError::BadRequest); } | ||
| let fields = project_custom_fields::Entity::find().filter(project_custom_fields::Column::ProjectId.eq(project_id)).filter(project_custom_fields::Column::Id.is_in(field_ids)).all(db).await?; | ||
| if fields.len() != unique_ids.len() { return Err(AppError::BadRequest); } | ||
| let field_map: HashMap<Uuid, project_custom_fields::Model> = fields.into_iter().map(|f| (f.id, f)).collect(); | ||
| for input in inputs { | ||
| let field = field_map.get(&input.field_id).ok_or(AppError::BadRequest)?; | ||
| match input.value.as_deref() { | ||
| None | Some("") => { | ||
| if field.is_required { return Err(AppError::BadRequest); } | ||
| task_custom_field_values::Entity::delete_many().filter(task_custom_field_values::Column::TaskId.eq(task_id)).filter(task_custom_field_values::Column::FieldId.eq(input.field_id)).exec(db).await?; | ||
| } | ||
| Some(value) => { | ||
| validate_custom_field_value(field, value)?; | ||
| let existing = task_custom_field_values::Entity::find().filter(task_custom_field_values::Column::TaskId.eq(task_id)).filter(task_custom_field_values::Column::FieldId.eq(input.field_id)).one(db).await?; | ||
| if let Some(row) = existing { | ||
| let mut active: task_custom_field_values::ActiveModel = row.into(); | ||
| active.value = Set(Some(value.to_string())); | ||
| active.update(db).await?; | ||
| } else { | ||
| task_custom_field_values::ActiveModel { task_id: Set(task_id), field_id: Set(input.field_id), value: Set(Some(value.to_string())) }.insert(db).await?; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
upsert 処理に効率性の問題と潜在的な競合状態があります。
問題点：
- Line 110: レコードが存在しない場合でも
DELETEクエリを毎回実行しており、無駄です。DELETEの前に存在確認を行うか、DELETEの結果を確認してから実行する方が効率的です。 - トランザクション制御がなく、複数のフィールドを更新中にエラーが発生すると、一部のみが更新された状態になります。
- 同じ task の同じ field に対する並行更新があると、race condition が発生する可能性があります（SELECT → UPDATE/INSERT の間に他のリクエストが介入）。
ON CONFLICT DO UPDATE (upsert) 構文を使用するか、トランザクションで囲むことを検討してください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/src/utils/custom_fields.rs` around lines 97 - 126, The
upsert_task_custom_field_values function currently issues unconditional DELETEs
per-empty value, lacks transaction boundaries and is vulnerable to race
conditions; fix by wrapping the whole operation in a DB transaction (begin a
transaction via the ConnectionTrait implementation) and replace the
SELECT→UPDATE/INSERT and unconditional DELETE logic on
task_custom_field_values::Entity / task_custom_field_values::ActiveModel with a
single upsert (INSERT ... ON CONFLICT (task_id, field_id) DO UPDATE SET value =
EXCLUDED.value) for non-empty values and a conditional DELETE only when a
matching row exists (or use a DELETE that returns affected rows) for empty
values, keeping validate_custom_field_value calls intact and referring to
input.field_id and task_id to form the conflict key so concurrent requests are
safe and partial updates are rolled back on error.
Source: Coding guidelines
|
|
||
| let get = app.get_with_session(&format!("{tasks_base}/{task_id}")).await; | ||
| assert_eq!(get.status(), StatusCode::OK); | ||
| assert!(get.json::<Value>().await.expect("json")["custom_field_values"].is_array()); |
There was a problem hiding this comment.
取得レスポンスの検証が弱く、空配列でも通過してしまいます。
Line 59 は配列型チェックのみのため、保存失敗で custom_field_values が空でも成功します。件数（2件）と field_id/value の一致まで検証してください。
差分案
- assert!(get.json::<Value>().await.expect("json")["custom_field_values"].is_array());
+ let body = get.json::<Value>().await.expect("json");
+ let values = body["custom_field_values"]
+ .as_array()
+ .expect("custom_field_values should be array");
+ assert_eq!(values.len(), 2);
+ assert!(values.iter().any(|v| v["field_id"] == number_id && v["value"] == "5"));
+ assert!(values.iter().any(|v| v["field_id"] == select_id && v["value"] == "m"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| assert!(get.json::<Value>().await.expect("json")["custom_field_values"].is_array()); | |
| let body = get.json::<Value>().await.expect("json"); | |
| let values = body["custom_field_values"] | |
| .as_array() | |
| .expect("custom_field_values should be array"); | |
| assert_eq!(values.len(), 2); | |
| assert!(values.iter().any(|v| v["field_id"] == number_id && v["value"] == "5")); | |
| assert!(values.iter().any(|v| v["field_id"] == select_id && v["value"] == "m")); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/tests/custom_fields_integration.rs` at line 59, The test only
asserts that custom_field_values is an array, which lets an empty array pass;
instead, parse the response JSON once (replace the inline
get.json::<Value>().await.expect("json") call with a let body =
get.json::<Value>().await.expect("json")) and assert that
body["custom_field_values"].as_array().map(|a| a.len()) == Some(2), then verify
each item's "field_id" and "value" match the expected pairs (compare strings/ids
for the two entries), e.g. iterate
body["custom_field_values"].as_array().unwrap() and assert field_id and value
equality for each expected record to ensure both count and content are
validated.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
project_custom_fields/task_custom_field_valuesテーブルを追加text/number/select/date/url/checkboxの6種類をサポートselect型はoptions外の値をバリデーションエラーとして拒否GET/POST /projects/{id}/custom-fields— フィールド定義の一覧・作成PATCH/DELETE /projects/{id}/custom-fields/{field_id}— フィールド定義の更新・削除GET/POST/PATCHにカスタムフィールド値の読み書きを統合関連仕様書:
docs/features/tasks/6.custom-fields.mdTest plan
POST /custom-fieldsで6種類すべてのフィールド型が作成できることselect型でoptions外の値を送ると 400 が返ることGET /tasks/{id}レスポンスにcustom_field_valuesが含まれることtask_custom_field_valuesも削除されること（CASCADE）cargo testが通ること🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
プロジェクトカスタムフィールド管理
タスクカスタムフィールド値の管理
テスト