Repository navigation
Release v0.9.5 SP1 + parser fix + CI fix - #40
Merged
Merged
Conversation
The constraint check in _parse_column_line_robust used str.startswith() against a tuple of keywords. That matched any column whose name merely began with one of them, so unique_code, constraint_name, check_flag and excluded_at were all treated as table constraints and silently dropped. The Schema Designer then drew a table with fewer columns than the SQL declares and gave no warning. The keywords are now matched with a word boundary so PRIMARY KEY (a, b), UNIQUE (email) and CONSTRAINT ... CHECK (...) are still skipped while prefixed column names parse as columns. Adds two tests. One fails on the old code with unique_code and constraint_name missing from the column list. The other pins down that table-level constraints are still skipped.
…from Sal-ami/fix/schema-parser-constraint-keyword-columns
The constraint check in `_parse_column_line_robust` used `str.startswith()` against a tuple of keywords. `startswith` matches prefixes, so any column whose name merely began with one of the keywords got treated as a table constraint and dropped.
This table loses three columns silently:
```sql
CREATE TABLE t (
id SERIAL PRIMARY KEY,
unique_code VARCHAR(10),
constraint_name VARCHAR(50),
check_flag BOOLEAN
);
```
Parsing it returns only `id`. The Schema Designer draws the table with a third of its columns missing and shows no error, so it looks like the SQL only ever declared one column.
The keywords are now matched with a word boundary. `PRIMARY KEY (a, b)`, `UNIQUE (email)` and `CONSTRAINT chk_len CHECK (...)` are still skipped, and prefixed column names parse as columns.
Two tests added:
- one covers the column names above and fails on the current code with `unique_code` and `constraint_name` missing from the result
- one pins down that table-level constraints are still skipped, so the fix cannot drift the other way
Verified locally with the four test files that do not need a live database:
```
pytest tests/test_schema_parser.py tests/test_schema_cache.py tests/test_settings.py tests/test_models.py
80 passed
```
`black --check` and `flake8` are clean on both changed files. mypy reports no issues on `schema_parser.py`. I could not run the database backed tests because there is no PostgreSQL server on my machine at this time.
The test database password was hardcoded as 'admin123' in tests/conftest.py. While this is a throw-away database for local and CI use, the value ended up in the repository history and could not be overridden without editing source. Now reads TEST_DB, TEST_USER, TEST_PASSWORD, TEST_HOST, and TEST_PORT from the environment, with the previous values as defaults so existing workflows (local + CI) keep working unchanged. Also added .env.example as documentation and .env to .gitignore, so contributors who point tests at a non-local PostgreSQL can keep the real credentials out of the repo. Reported-by: <GitHub handle of the reviewer>
The test database password was hardcoded as 'admin123' in tests/conftest.py. While this is a throw-away database for local and CI use, the value ended up in the repository history and could not be overridden without editing source. Now reads TEST_DB, TEST_USER, TEST_PASSWORD, TEST_HOST, and TEST_PORT from the environment, with the previous values as defaults so existing workflows (local + CI) keep working unchanged. Also added .env.example as documentation and .env to .gitignore, so contributors who point tests at a non-local PostgreSQL can keep the real credentials out of the repo. Reported-by: [Sal-ami](https://github.com/Sal-ami)
The test database password was hardcoded as 'admin123' in tests/conftest.py. While this is a throw-away database for local and CI use, the value ended up in the repository history and could not be overridden without editing source. Now reads TEST_DB, TEST_USER, TEST_PASSWORD, TEST_HOST, and TEST_PORT from the environment, with the previous values as defaults so existing workflows (local + CI) keep working unchanged. Also added .env.example as documentation and .env to .gitignore, so contributors who point tests at a non-local PostgreSQL can keep the real credentials out of the repo. Reported-by: [Sal-ami](https://github.com/Sal-ami)
Peter-L-SVK
added
enhancement
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
2 participants
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
Merge
stageintomain. This brings:areas, and 212 unit tests
Tag:
v0.9.5-sp1Changes Since SP1
fix(parser): stop dropping columns named like constraint keywords
Contributed by @Sal-ami.
The constraint check in
_parse_column_line_robustusedstr.startswith()against a tuple of keywords. That matched anycolumn whose name merely began with one of them, so
unique_code,constraint_name,check_flag, andexcluded_atwere all treatedas table constraints and silently dropped. The Schema Designer
then drew a table with fewer columns than the SQL declared and gave
no warning.
The keywords are now matched with a word boundary, so
PRIMARY KEY (a, b),UNIQUE (email), andCONSTRAINT ... CHECK (...)arestill skipped while prefixed column names parse as columns.
Adds two tests: one that fails on the old code with
unique_codeand
constraint_namemissing, and one that pins down thattable-level constraints are still skipped.
fix(ci): use GitHub secret for postgres service password
The postgres service container password is now read from the
repository secret
TEST_DB_PASSWORD, and the same secret is passedto the pytest step via the
TEST_PASSWORDenvironment variable sotests/conftest.pycan pick it up.Verification