-
Notifications
You must be signed in to change notification settings - Fork 1
cowork-bot: emit valid SQL for mixed-type columns and empty/nested-only roots #34
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
Open
Coding-Dev-Tools
wants to merge
1
commit into
main
Choose a base branch
from
cowork/improve-json2sql-2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+259
−35
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # Seeded by the repo-improver-rotation Cowork job into cowork/improve-* branches. | ||
| # Opens a PR automatically when such a branch is pushed (sandbox cannot reach | ||
| # the GitHub API directly; this runs server-side with the repo's GITHUB_TOKEN). | ||
| name: cowork-auto-pr | ||
| on: | ||
| push: | ||
| branches: ['cowork/improve-**'] | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| jobs: | ||
| ensure-pr: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| # gh pr create requires a local git checkout to diff head against base; | ||
| # without this step every run failed with "not a git repository" and no | ||
| # PR was ever opened (fleet-wide defect: 11/11 seeded copies lacked it). | ||
| - name: Check out the pushed branch | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| ref: ${{ github.ref_name }} | ||
| fetch-depth: 0 | ||
| - name: Open PR for this branch if none exists | ||
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| run: | | ||
| set -eu | ||
| existing=$(gh pr list --repo "$GITHUB_REPOSITORY" --head "$GITHUB_REF_NAME" --state open --json number --jq 'length') | ||
| if [ "$existing" = "0" ]; then | ||
| gh pr create --repo "$GITHUB_REPOSITORY" \ | ||
| --head "$GITHUB_REF_NAME" \ | ||
| --title "cowork-bot: automated improvements ($GITHUB_REF_NAME)" \ | ||
| --body "Automated improvement PR from the Cowork repo-improver rotation (one coherent senior-dev improvement per run; see individual commit messages). Subsequent runs push additional commits to this PR rather than opening new ones." | ||
| else | ||
| echo "Open PR already exists for $GITHUB_REF_NAME — nothing to do." | ||
| fi |
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| """Regression tests for type inference and valid-SQL edge cases. | ||
|
|
||
| These guard the two correctness fixes in converter.py: | ||
|
|
||
| 1. Mixed-type columns collapse to TEXT so the generated INSERTs are valid for | ||
| every dialect (Postgres/MySQL would reject a quoted string literal placed | ||
| into a numeric column). | ||
| 2. Empty / nested-only roots emit valid SQL (or an explicit comment) instead of | ||
| an invalid ``CREATE TABLE "x" ();`` / ``INSERT INTO "x" () VALUES ();``. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| import re | ||
|
|
||
| import pytest | ||
|
|
||
| from json2sql.converter import JSONToSQLConverter | ||
| from json2sql.dialects import Dialect | ||
|
|
||
| NUMERIC = {"INT", "INTEGER", "DOUBLE PRECISION", "DOUBLE", "REAL"} | ||
|
|
||
|
|
||
| def column_type(out: str, col: str, dialect: Dialect) -> str | None: | ||
| """Extract the SQL type declared for ``col`` from a CREATE TABLE in ``out``.""" | ||
| qcol = f"`{col}`" if dialect == Dialect.MYSQL else f'"{col}"' | ||
| m = re.search(re.escape(qcol) + r"\s+(\w+(?:\([^)]*\))?)", out) | ||
| return m.group(1) if m else None | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("dialect", [Dialect.POSTGRES, Dialect.MYSQL, Dialect.SQLITE]) | ||
| def test_mixed_string_then_int_collapses_to_text(dialect): | ||
| """A string followed by an int must become TEXT, not a numeric type.""" | ||
| out = JSONToSQLConverter(dialect=dialect).convert( | ||
| json.dumps([{"k": "hello"}, {"k": 100}]) | ||
| ) | ||
| assert column_type(out, "k", dialect) == "TEXT" | ||
| assert "'hello'" in out and "100" in out | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("dialect", [Dialect.POSTGRES, Dialect.MYSQL, Dialect.SQLITE]) | ||
| def test_mixed_int_then_string_collapses_to_text(dialect): | ||
| """An int followed by a string must also become TEXT (order independence).""" | ||
| out = JSONToSQLConverter(dialect=dialect).convert( | ||
| json.dumps([{"k": 1}, {"k": "hello"}]) | ||
| ) | ||
| assert column_type(out, "k", dialect) == "TEXT" | ||
|
|
||
|
|
||
| def test_mixed_int_then_float_collapses_to_text(): | ||
| """Two incompatible concrete types also collapse to TEXT.""" | ||
| out = JSONToSQLConverter().convert(json.dumps([{"k": 1}, {"k": 2.5}])) | ||
| assert column_type(out, "k", Dialect.POSTGRES) == "TEXT" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("dialect", [Dialect.POSTGRES, Dialect.MYSQL, Dialect.SQLITE]) | ||
| def test_null_then_int_upgrades_to_int(dialect): | ||
| """A NULL first does not pin the column to TEXT; a later int wins.""" | ||
| out = JSONToSQLConverter(dialect=dialect).convert( | ||
| json.dumps([{"k": None}, {"k": 42}]) | ||
| ) | ||
| assert column_type(out, "k", dialect) in NUMERIC | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("dialect", [Dialect.POSTGRES, Dialect.MYSQL, Dialect.SQLITE]) | ||
| def test_int_then_null_stays_int(dialect): | ||
| """A trailing NULL must not downgrade an established concrete type.""" | ||
| out = JSONToSQLConverter(dialect=dialect).convert( | ||
| json.dumps([{"k": 7}, {"k": None}]) | ||
| ) | ||
| assert column_type(out, "k", dialect) in NUMERIC | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("dialect", [Dialect.POSTGRES, Dialect.MYSQL, Dialect.SQLITE]) | ||
| def test_all_null_column_defaults_to_text(dialect): | ||
| """An all-NULL column resolves to TEXT rather than an unset/empty type.""" | ||
| out = JSONToSQLConverter(dialect=dialect).convert( | ||
| json.dumps([{"k": None}, {"k": None}]) | ||
| ) | ||
| assert column_type(out, "k", dialect) == "TEXT" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("dialect", [Dialect.POSTGRES, Dialect.MYSQL, Dialect.SQLITE]) | ||
| def test_null_then_string_becomes_text(dialect): | ||
| """NULL first, then a string -> a text column (not an unset numeric type). | ||
|
|
||
| A NULL is "unset", so the later string wins; MySQL renders this as | ||
| VARCHAR(255), Postgres/SQLite as TEXT. The key invariant is that it is a | ||
| string type, never INTEGER/INT. | ||
| """ | ||
| out = JSONToSQLConverter(dialect=dialect).convert( | ||
| json.dumps([{"k": None}, {"k": "x"}]) | ||
| ) | ||
| assert column_type(out, "k", dialect) in {"TEXT", "VARCHAR(255)"} | ||
|
|
||
|
|
||
| def test_empty_object_root_emits_no_invalid_sql(): | ||
| """An empty object must not produce ``CREATE TABLE "data" ();``.""" | ||
| out = JSONToSQLConverter().convert(json.dumps({})) | ||
| assert "CREATE TABLE" not in out | ||
| assert "INSERT INTO" not in out | ||
| # An explicit, observable comment instead of a silent empty string. | ||
| assert out.startswith("-- ") | ||
|
|
||
|
|
||
| def test_generate_schema_empty_object_emits_comment(): | ||
| """generate_schema on an empty object returns a comment, not invalid SQL.""" | ||
| out = JSONToSQLConverter().generate_schema(json.dumps({})) | ||
| assert "CREATE TABLE" not in out | ||
| assert out.startswith("-- ") | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("dialect", [Dialect.POSTGRES, Dialect.MYSQL, Dialect.SQLITE]) | ||
| def test_nested_array_only_root_keeps_child_tables_only(dialect): | ||
| """Flatten of a root with only a nested array emits child table, no bad root.""" | ||
| out = JSONToSQLConverter(dialect=dialect, flatten=True).convert( | ||
| json.dumps({"items": [{"x": 1}, {"x": 2}]}) | ||
| ) | ||
| assert "data_items" in out | ||
| assert column_type(out, "x", dialect) in NUMERIC | ||
| # The bogus empty root CREATE/INSERT must be absent. | ||
| assert 'CREATE TABLE "data" (\n\n);' not in out | ||
| assert "INSERT INTO \"data\" ()" not in out | ||
|
|
||
|
|
||
| def test_convert_never_emits_empty_column_list(): | ||
| """No generated statement may contain a zero-column table or insert.""" | ||
| out = JSONToSQLConverter(flatten=True).convert( | ||
| json.dumps([{"a": 1, "b": [{"c": 2}]}, {"a": 3}]) | ||
| ) | ||
| assert "();" not in out | ||
| assert "INSERT INTO" in out |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When flattening input such as
{"items": [{}]}, this guard suppresses only the empty root table._flatten_nestedstill addsdata_itemswith an emptycolumnsmapping, andconvert()subsequently emits bothCREATE TABLE "data_items" ();andINSERT INTO "data_items" () VALUES ();, which are invalid SQL. Skip child tables with no scalar columns as well.Useful? React with 👍 / 👎.