Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backend: fix v2_job.tag length #5242

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

uael
Copy link
Collaborator

@uael uael commented Feb 7, 2025

Important

Adds migration to change tag column to VARCHAR(255) in multiple tables and handles view recreation.

  • Database Migration:
    • Adds 20250205131516_v2_job_tag_size.up.sql to alter tag column to VARCHAR(255) in v2_job, flow, schedule, and script tables.
    • Stores, drops, and recreates views job, v2_as_queue, v2_as_completed_job during migration.
    • Adds empty 20250205131516_v2_job_tag_size.down.sql for down migration.

This description was created by Ellipsis for 4b17be8. It will automatically update as commits are pushed.

@uael uael requested a review from rubenfiszel as a code owner February 7, 2025 15:24
Copy link

cloudflare-workers-and-pages bot commented Feb 7, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 4b17be8
Status: ✅  Deploy successful!
Preview URL: https://bebd8ef3.windmill.pages.dev
Branch Preview URL: https://uael-fix-v2-job-tag-length.windmill.pages.dev

View logs

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 4d34fab in 1 minute and 5 seconds

More details
  • Looked at 44 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. backend/migrations/20250205131516_v2_job_tag_size.up.sql:5
  • Draft comment:
    Initialize arrays to avoid null issues
  • Reason this comment was not posted:
    Marked as duplicate.
2. backend/migrations/20250205131516_v2_job_tag_size.up.sql:25
  • Draft comment:
    Check ALTER TABLE conversion
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 20% <= threshold 50%
    The comment is vague and does not provide a specific suggestion or question. It seems to be asking the author to double-check something without providing guidance or a specific issue to address. It doesn't align with the rules for good comments.
3. backend/migrations/20250205131516_v2_job_tag_size.up.sql:5
  • Draft comment:
    Initialize 'view_names' and 'view_definitions' to empty arrays to avoid null issues with array_append and array_length.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, providing guidance on why certain variables are initialized in a specific way. It doesn't suggest a change or ask for clarification on the code's intent. Therefore, it violates the rule against making purely informative comments.
4. backend/migrations/20250205131516_v2_job_tag_size.up.sql:25
  • Draft comment:
    Altering the column to VARCHAR(255) can fail if existing tag values exceed 255 characters. Verify data length or use a USING clause to handle truncation.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    This is a valid concern about data integrity during schema migration. However, PostgreSQL will actually fail the migration by default if any data would be truncated, so this is a built-in safety mechanism. The comment is suggesting verification that's already handled by the database system. Additionally, without seeing the previous column type, we can't know if this is even a reduction in size.
    I might be wrong about the previous column type - what if it was TEXT and this is indeed a reduction? Also, some configurations might allow silent truncation.
    Even if this is a reduction from TEXT, PostgreSQL's default behavior is to error on data loss, making this a self-checking operation. If silent truncation was configured, it would be a system-wide setting that should be addressed separately.
    Delete the comment because PostgreSQL handles this safety check by default, and the comment is essentially asking for verification of something the database system already enforces.

Workflow ID: wflow_mLAKNmq3Y8R7Nw64


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -0,0 +1 @@
-- Add down migration script here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Down migration script is just a placeholder. Provide a proper down migration to reverse the changes.

@uael uael force-pushed the uael/fix_v2_job_tag_length branch from 4d34fab to 4b17be8 Compare February 7, 2025 15:34
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on 4b17be8 in 1 minute and 14 seconds

More details
  • Looked at 47 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. backend/migrations/20250205131516_v2_job_tag_size.up.sql:5
  • Draft comment:
    Initialize arrays to an empty state to avoid appending to NULL.
  • Reason this comment was not posted:
    Marked as duplicate.
2. backend/migrations/20250205131516_v2_job_tag_size.up.sql:20
  • Draft comment:
    Handle potential NULL from array_length when no views are found.
  • Reason this comment was not posted:
    Marked as duplicate.
3. backend/migrations/20250205131516_v2_job_tag_size.up.sql:5
  • Draft comment:
    Initialize view_definitions and view_names to empty arrays (e.g. '{}') to avoid null issues with array_append.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    While technically correct that NULL arrays could cause issues with array_append, PostgreSQL automatically converts NULL arrays to empty arrays when using array_append. The code as written is safe and follows common PostgreSQL patterns. The suggestion would make the code more explicit but doesn't fix any actual problem.
    The comment points out a theoretical edge case that could cause issues in some contexts. Maybe there's a specific reason they want explicit initialization for clarity or consistency?
    PostgreSQL's array_append function is specifically designed to handle NULL arrays safely by converting them to empty arrays. This is well-documented behavior that developers rely on.
    The comment should be deleted as it suggests a change that isn't necessary - the code is already safe as written and follows standard PostgreSQL patterns.
4. backend/migrations/20250205131516_v2_job_tag_size.up.sql:25
  • Draft comment:
    Ensure that existing 'tag' data does not exceed 255 characters to avoid ALTER TABLE failures; consider using a USING clause or data validation if needed.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    This is a valid concern about data integrity during schema migration. However, the comment starts with "Ensure that..." which violates our rules. Additionally, if there was a risk of data truncation, the migration would fail automatically - Postgres won't silently truncate data. The suggestion about USING clause isn't necessary since we're not changing data format.
    The comment raises a legitimate database migration concern. Data truncation could cause production issues if not handled properly.
    While data integrity is important, Postgres will automatically prevent data truncation by failing the migration if any values exceed 255 chars. No explicit validation is needed.
    Delete the comment. It starts with "Ensure that..." which violates our rules, and the concern is automatically handled by Postgres's ALTER TABLE behavior.

Workflow ID: wflow_VWsTwGFxSrvMTMMF


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -0,0 +1 @@
-- Add down migration script here
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Down migration script is empty; please implement reversal logic.

@rubenfiszel rubenfiszel force-pushed the main branch 3 times, most recently from 30f0807 to 9c57565 Compare February 14, 2025 03:38
@rubenfiszel rubenfiszel force-pushed the main branch 9 times, most recently from 0d0ada6 to 1ef482e Compare February 18, 2025 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant