fix: restore E2E stability and address regressions (#11887)#11996
fix: restore E2E stability and address regressions (#11887)#11996purvanshjoshi wants to merge 9 commits intocBioPortal:masterfrom
Conversation
…ns, and missing DB version sync
There was a problem hiding this comment.
Pull request overview
This PR attempts to restore E2E test stability and fix regressions after PR #11970 was merged. The main objectives are to fix NullPointerExceptions in NamespaceAttributeServiceImpl, restore CancerStudyPermissionEvaluator logic for DOWNLOAD permissions, synchronize database schema version to 2.14.6, and restore missing GitHub Actions E2E workflows.
Changes:
- Added null safety guards to NamespaceAttributeServiceImpl to prevent NullPointerExceptions when repository methods return null
- Introduced DOWNLOAD permission level with logic in CancerStudyPermissionEvaluator to check download groups, superuser permissions, and global download group configuration
- Updated database version from 2.14.5 to 2.14.6 in pom.xml, cgds.sql, and migration.sql
- Restored GitHub Actions workflows for integration tests and security integration tests
- Added downloadGroups field to CancerStudy model and corresponding MyBatis mappers
- Added unit tests for null handling in NamespaceAttributeServiceImpl and download permission logic in CancerStudyPermissionEvaluator
- Updated .gitignore to exclude temporary test files and agent directories
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/cbioportal/legacy/service/impl/NamespaceAttributeServiceImpl.java | Added null guards for repository return values to prevent NullPointerException |
| src/test/java/org/cbioportal/legacy/service/impl/NamespaceAttributeServiceImplTest.java | Added unit tests for null handling scenarios |
| src/main/java/org/cbioportal/legacy/utils/security/AccessLevel.java | Added DOWNLOAD enum value |
| src/main/java/org/cbioportal/application/security/CancerStudyPermissionEvaluator.java | Implemented DOWNLOAD permission logic with downloadGroups checks and superuser handling |
| src/test/java/org/cbioportal/application/security/CancerStudyPermissionEvaluatorTest.java | Added unit tests for download permission evaluation |
| src/main/java/org/cbioportal/application/security/config/MethodSecurityConfig.java | Added downloadGroup parameter to CancerStudyPermissionEvaluator configuration |
| src/main/java/org/cbioportal/legacy/model/CancerStudy.java | Added downloadGroups field with getter/setter |
| src/main/resources/org/cbioportal/legacy/persistence/mybatis/StudyMapper.xml | Added download_groups column to SELECT query |
| src/main/resources/org/cbioportal/legacy/persistence/mybatis/SecurityMapper.xml | Added download_groups column to getCancerStudyGroups query |
| pom.xml | Updated db.version from 2.14.5 to 2.14.6 |
| src/main/resources/db-scripts/cgds.sql | Updated DB_SCHEMA_VERSION to 2.14.6 |
| src/main/resources/db-scripts/migration.sql | Updated DB_SCHEMA_VERSION to 2.14.6 |
| src/test/resources/cgds.sql | Updated DB_SCHEMA_VERSION to 2.14.6 |
| .github/workflows/integration-test.yml | Restored E2E integration test workflow |
| .github/workflows/security-integration-test.yml | Restored security integration test workflow |
| .gitignore | Added patterns for temporary test files and agent directories |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/org/cbioportal/application/security/CancerStudyPermissionEvaluator.java
Outdated
Show resolved
Hide resolved
src/test/java/org/cbioportal/application/security/CancerStudyPermissionEvaluatorTest.java
Show resolved
Hide resolved
src/main/java/org/cbioportal/application/security/CancerStudyPermissionEvaluator.java
Show resolved
Hide resolved
src/main/resources/org/cbioportal/legacy/persistence/mybatis/SecurityMapper.xml
Show resolved
Hide resolved
src/main/resources/org/cbioportal/legacy/persistence/mybatis/StudyMapper.xml
Outdated
Show resolved
Hide resolved
…DOWNLOAD_GROUPS column, fix SecurityMapper, upgrade CI checkout action, add 3 security unit tests
|
Cc: @inodb @alisman @dippindots I’ve updated this PR to address the recent CI regressions and include hardening based on the Copilot review. Notably, I've implemented a fix for the ClickHouse E2E startup crash (Column 'download_groups' not found). As @onursumer mentioned in #11886, the E2E tests are currently using a remote DB hotfix; my change in StudyMapper.xml Summary of hardening & stability fixes: Resolved E2E Crash: Removed download_groups from StudyMapper SELECT to support the current "remote DB" CI state. CancerStudyPermissionEvaluator.java SecurityMapper.xml.Schema Sync: Updated DDL scripts (cgds.sql and migration.sql) to v2.14.6. Ready for a fresh review!" |
… from StudyMapper
…ping for user authorities
367cdfb to
aad6ae0
Compare
|
Hi @alisman, @inodb, @dippindots, and @onursumer, I’ve pushed updates to address the recent CI regressions and added hardening based on the Copilot review. All unit tests, security tests, and frontend builds are now passing successfully! ✅ However, the run_e2e_localdb_tests job on CircleCI is still failing. I’ve already implemented several stability fixes (such as null-guards for NamespaceAttributeServiceImpl, AlterationEnrichmentController Could you kindly review the current changes and provide some guidance or point me in the right direction to resolve these remaining E2E test failures? Any insights into the specific test environment setup or the failure logs would be greatly appreciated so I can get this green. Thanks! |
[PR Description] fix: restore E2E stability and address regressions (#11887)
Fix #11887
Overview
This Pull Request restores stability to the cBioPortal End-to-End (E2E) and integration test suites. Following the closure of PR #11970, several critical stability fixes and configuration synchronizations were missing from the
masterbranch, leading to widespread CI failures across all open PRs. This PR incorporates those missing changes and hardens the codebase against common NullPointerExceptions (NPEs) and permission logic regressions.Proposed Changes
1. Stability & NPE Mitigation
NullPointerExceptionthat was causing deployment failures in test environments when studies or attributes were partially loaded.DOWNLOADpermission level to the project's security utilities to facilitate granular control over data export actions.2. Enhanced Permission Evaluation
ALL_CANCER_STUDIES) at the start of permission checks. This significantly improves performance and ensures that administrators always have consistent access across all study levels.download_groupsfeature. Users can now be granted specific access to download study data independently of their read permissions, matching the intended design for restricted data sets.3. Database & CI Synchronization
db.versionto2.14.6across the pom.xml and all core SQL scripts (cgds.sql for main/test and migration.sql). This ensures full compatibility with the schema expectations of the restoration and integration scripts used in CI.integration-test.ymlandsecurity-integration-test.ymlto the repository's.github/workflowsdirectory. These files are essential for providing the necessary CI visibility that was lost during previous merges.4. Data Model & Persistence
downloadGroupsproperty and corresponding accessors to theCancerStudymodel.StudyMapper.xmlandSecurityMapper.xmlto include thedownload_groupscolumn in SQL queries for both MySQL and Clickhouse backends.Quality Assurance & Verification
Unit Testing
Restored and synchronized critical unit test suites that were missing or out-of-date:
CancerStudyPermissionEvaluatorTest: Verified 100% pass rate for the new permission evaluation logic, specifically coveringDOWNLOADlevel access, superuser overrides, and group-based restrictions.NamespaceAttributeServiceImplTest: Verified the effectiveness of the new null guards in preventing regressions during attribute fetching.Local Build
Verified that the project compiles successfully via Maven (
mvn clean compile) with all new model and security changes.Criticality
These fixes are critical for unblocking the entire cBioPortal development pipeline. Currently, almost every open PR is failing CI due to the regressions addressed here. Merging this PR will restore a healthy state to the
masterbranch and allow other contributions to be properly validated.Notify Reviewers
@alisman
@inodb
@dippindots
@onursumer