-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix glossary status #25428
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
base: main
Are you sure you want to change the base?
fix glossary status #25428
Conversation
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
.../src/main/resources/ui/src/components/Glossary/GlossaryTermTab/GlossaryTermTab.component.tsx
Show resolved
Hide resolved
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ListFilter.java
Outdated
Show resolved
Hide resolved
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
|
@gitar-bot fix the SQL injection, add a commit |
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
I've fixed the SQL injection vulnerability in The fix validates input values against the Changes in GlossaryTermRepository.java:1633
This complements the earlier fix in |
db7c661 to
3038077
Compare
🔍 CI failure analysis for cdbe7e5: Maven SonarCloud CI shows IDENTICAL failures as Maven PostgreSQL CI - confirms consistent Apps/AWS credential issues, UNRELATED (7804 tests, 1 failed, 3 errors, 99.95% pass rate)IssueMaven SonarCloud CI (job 61413375919) failed with IDENTICAL results to Maven PostgreSQL CI. Root CauseUNRELATED to this PR - Same Apps and AWS credential test failures across both Maven CI environments Test Results: 7804 tests, 1 failed, 3 errors, 701 skipped (99.95% pass rate) Key Finding - Identical Results Across Maven CI Environments:
Same Failed Tests (Both Environments):
Historical Pattern - Multiple Maven CI RunsPrevious runs documented:
Current runs:
Proves: Consistent pre-existing failures in Apps and AWS credential functionality Evidence This Is UNRELATED
AssessmentUNRELATED pre-existing issue. Having identical failures across both Maven PostgreSQL and SonarCloud CI environments confirms these are pre-existing bugs in Apps and AWS credential functionality, not introduced by this PR's glossary term status filtering changes. The glossary term status filtering implementation is working correctly with 99.95% Maven test pass rate. Code Review 👍 Approved with suggestions 19 resolved / 22 findingsGood implementation of glossary status filtering with comprehensive tests and proper SQL injection prevention. Three minor issues from previous review remain unresolved: missing newlines in SQL files, duplicated parseEntityStatusValues method, and MySQL migration lacking IF NOT EXISTS. 💡 Edge Case: MySQL migration missing IF NOT EXISTS for column addition📄 bootstrap/sql/migrations/native/1.11.7/mysql/schemaChanges.sql:3-5 📄 bootstrap/sql/migrations/native/1.11.7/postgres/schemaChanges.sql:3-5 📄 bootstrap/sql/migrations/native/1.11.7/mysql/schemaChanges.sql 📄 bootstrap/sql/migrations/native/1.11.7/postgres/schemaChanges.sql The MySQL migration script uses a bare ALTER TABLE glossary_term_entity
ADD COLUMN entityStatus VARCHAR(32)While the PostgreSQL migration correctly uses ALTER TABLE glossary_term_entity
ADD COLUMN IF NOT EXISTS entityStatus VARCHAR(32)If the MySQL migration is run twice (e.g., during a failed migration retry or in certain deployment scenarios), it will fail with a "Duplicate column name" error. MySQL 8.0 doesn't support Note: The PostgreSQL migration handles this correctly with Suggested fix: Consider using a conditional check pattern for MySQL, such as: -- Using stored procedure or checking if column exists first
SELECT COUNT(*) INTO @col_exists FROM information_schema.columns
WHERE table_schema = DATABASE() AND table_name = 'glossary_term_entity' AND column_name = 'entityStatus';
SET @query = IF(@col_exists = 0, 'ALTER TABLE glossary_term_entity ADD COLUMN entityStatus VARCHAR(32) GENERATED ALWAYS AS (json ->> ''$.entityStatus'') STORED', 'SELECT 1');
PREPARE stmt FROM @query;
EXECUTE stmt;Or wrap in a stored procedure that checks for column existence first. 💡 Edge Case: Missing newline at end of migration SQL files📄 bootstrap/sql/migrations/native/1.11.7/mysql/schemaChanges.sql:9 📄 bootstrap/sql/migrations/native/1.11.7/postgres/schemaChanges.sql:9 📄 bootstrap/sql/migrations/native/1.11.7/mysql/schemaChanges.sql:15 Both the MySQL and PostgreSQL migration SQL files are missing a newline at the end of the file. While this is a minor issue, many code conventions and tools (like POSIX standards, git diffs, and various linters) expect files to end with a newline character. The files currently end with the CREATE INDEX statement without a trailing newline, which can cause issues with file concatenation tools and may result in "\ No newline at end of file" warnings in diffs. Suggested fix: Add a newline at the end of both:
💡 Quality: Duplicated parseEntityStatusValues method across classes📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ListFilter.java:165-176 📄 openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java:1632-1643 The
Both methods have the exact same logic:
This violates DRY (Don't Repeat Yourself) principle and creates maintenance burden - if the parsing logic needs to change, it must be updated in multiple places. Suggested fix: Extract this common method to a shared utility class (e.g., ✅ 19 resolved✅ Bug: Comment typo: migration version 1.1.7 should be 1.11.7
✅ Bug: Comment typo: migration version 1.1.7 should be 1.11.7
✅ Security: SQL injection via string concatenation in IN clause
✅ Edge Case: Missing dependency in useEffect may cause stale closures
✅ Quality: Duplicated status validation logic in two places
...and 14 more resolved from earlier reviews Tip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
Screen.Recording.2026-01-21.at.4.53.10.PM.mov
Thank you for your contribution!
Unless your change is trivial, please create an issue to discuss the change before creating a PR.
-->
Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
entityStatuscolumn with index to glossary terms table for efficient status-based filteringListFilter.getEntityStatusCondition()to use IN clause instead of equality for multi-status queriesGlossaryTermRepository.searchGlossaryTermsInternalto support filtering by multiple status valuesGlossaryTermTab.component.tsxwith multi-select status filter controlsglossaryAPI.tsto pass status filters to backend endpointsGlossaryTermResourceIT.javaintegration tests for status filtering scenariosGlossaryStatusFilterLargeDataset.spec.tsandGlossaryStatusFilterNestedTerms.spec.tsThis will update automatically on new commits.