Skip to content

chore(core): prevent authorization error immediately after table created#6337

Merged
bluestreak01 merged 1 commit intomasterfrom
ia_fix_ilp_create_table
Oct 31, 2025
Merged

chore(core): prevent authorization error immediately after table created#6337
bluestreak01 merged 1 commit intomasterfrom
ia_fix_ilp_create_table

Conversation

@glasstiger
Copy link
Copy Markdown
Contributor

@glasstiger glasstiger commented Oct 30, 2025

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Oct 30, 2025 •
edited
Loading

Walkthrough

The changes refactor the table and materialized view creation flow by moving DDL listener notification responsibility from callers into the createTableOrMatViewUnsecure method itself. The method signature now requires SecurityContext and tableKind parameters. A new test validates DDL listener callbacks during HTTP-based table creation.

Changes

Cohort / File(s) Summary
Table Creation Refactor
core/src/main/java/io/questdb/cairo/CairoEngine.java		 Refactored createTableOrMatViewUnsecure method to accept SecurityContext and tableKind parameters; moved DDL listener notification from callers into the method via getDdlListener(tableToken).onTableOrMatViewCreated(securityContext, tableToken, tableKind)
Test Coverage
core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java		 Added new test testDdlListenerOnTableCreated to validate DDL listener callbacks, table metadata, visibility constraints, and concurrent lock behavior during HTTP-based table creation

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Verify all call sites of createTableOrMatViewUnsecure have been updated with the new SecurityContext and tableKind parameters (the summary indicates multiple callers exist but their updates are not fully detailed in the provided diff)
  • Confirm the tableKind parameter is correctly propagated through the creation path (e.g., TABLE_KIND_REGULAR_TABLE usage)
  • Validate that DDL listener notification occurs at the correct point in the creation flow and that callers no longer invoke it separately
  • Review the test logic to ensure it properly validates DDL listener behavior, concurrent access constraints, and locking semantics

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "chore(core): prevent authorization error immediately after table created" is directly related to the main changes in this pull request. The refactoring moves DDL listener notification into the createTableOrMatViewUnsecure method and adds a SecurityContext parameter, which directly addresses preventing authorization errors by ensuring security context is properly propagated immediately after table creation. While the title describes the problem statement rather than the technical solution (method refactoring and parameter changes), it clearly and specifically summarizes the primary intent of the changeset.
Description Check ✅ Passed The pull request description, while minimal, is related to the changeset through its reference to the requiring pull request in the enterprise repository. It indicates that this change addresses a requirement from another PR (#768), which establishes a connection between the description and the actual modifications. Although the description lacks detailed explanation of what the code changes accomplish, it is not off-topic or misleading, and the check is described as very lenient.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ia_fix_ilp_create_table

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@glasstiger
Copy link
Copy Markdown
Contributor Author

glasstiger commented Oct 30, 2025

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Oct 30, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai[bot]
coderabbitai bot reviewed Oct 30, 2025
View reviewed changes
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3e8a461 and e6792e2.

📒 Files selected for processing (2)
  • core/src/main/java/io/questdb/cairo/CairoEngine.java (4 hunks)
  • core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
  • GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-x64-zfs)
  • GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests on linux-arm64)
  • GitHub Check: New pull request (SelfHosted Other tests on linux-arm64)
  • GitHub Check: New pull request (SelfHosted Other tests on linux-x64-zfs)
  • GitHub Check: New pull request (Hosted Running tests on windows-other)
  • GitHub Check: New pull request (Hosted Running tests on windows-pgwire)
  • GitHub Check: New pull request (Hosted Running tests on windows-cairo)
  • GitHub Check: New pull request (Hosted Running tests on windows-fuzz2)
  • GitHub Check: New pull request (Hosted Running tests on windows-fuzz1)
  • GitHub Check: New pull request (Hosted Running tests on windows-griffin)
  • GitHub Check: New pull request (Hosted Running tests on mac-other)
  • GitHub Check: New pull request (Hosted Running tests on mac-pgwire)
  • GitHub Check: New pull request (Hosted Running tests on mac-cairo-fuzz)
  • GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start X64Zfs Agent)
  • GitHub Check: New pull request (Hosted Running tests on mac-cairo)
  • GitHub Check: New pull request (SelfHosted Griffin And Fuzz tests Start ARM Agent)
  • GitHub Check: New pull request (SelfHosted Other tests Start ARM Agent)
  • GitHub Check: New pull request (Rust Test and Lint on linux-jdk17)
  • GitHub Check: New pull request (SelfHosted Other tests Start X64Zfs Agent)
  • GitHub Check: New pull request (Hosted Running tests on mac-griffin)
  • GitHub Check: New pull request (Hosted Running tests with cover on linux-other)
  • GitHub Check: New pull request (Hosted Running tests with cover on linux-pgwire)
  • GitHub Check: New pull request (Hosted Running tests with cover on linux-cairo)
  • GitHub Check: New pull request (Hosted Running tests with cover on linux-fuzz)
  • GitHub Check: New pull request (Hosted Running tests with cover on linux-griffin)
  • GitHub Check: New pull request (Check Changes Check changes)
  • GitHub Check: build
  • GitHub Check: build
🔇 Additional comments (5)
core/src/main/java/io/questdb/cairo/CairoEngine.java (3)

571-571: LGTM!

The updated call correctly passes both securityContext and tableKind parameters to the refactored method.

1658-1668: LGTM!

The method signature correctly adds SecurityContext and tableKind parameters to support internal DDL listener notification with proper context.

1715-1716: LGTM - DDL notification timing is intentional.

The DDL listener is notified before the table is registered in the name registry (line 1716). This ensures authorization setup completes before the table becomes visible to other threads, preventing the authorization errors mentioned in the PR objectives. The table remains locked during this period for thread safety.

core/src/test/java/io/questdb/test/cutlass/http/line/LineHttpSenderTest.java (2)

32-32: LGTM!

The new imports (DefaultDdlListener, SecurityContext, AtomicBoolean) are necessary for the new test and properly utilized.

Also applies to: 35-35, 67-67

880-922: Well-designed test validating DDL listener timing.

The test correctly validates that the DDL listener is invoked before the table becomes visible to other threads, which is the core behavior change. The assertions confirm:

  • Table metadata is correct during the callback
  • Table is not yet registered (getTableTokenIfExists returns null)
  • Table name cannot be locked by competing threads

Minor consideration: Line 891 uses getCurrentId() which assumes no concurrent table creation. This is likely safe in the test environment but could be fragile if test isolation changes.

@glasstiger
Copy link
Copy Markdown
Contributor Author

glasstiger commented Oct 30, 2025

[PR Coverage check]

😍 pass : 3 / 3 (100.00%)

file detail

path covered line new line coverage
🔵 io/questdb/cairo/CairoEngine.java 3 3 100.00%

@bluestreak01 bluestreak01 merged commit 00d8166 into master Oct 31, 2025
34 checks passed
@bluestreak01 bluestreak01 deleted the ia_fix_ilp_create_table branch October 31, 2025 01:43
@coderabbitai coderabbitai bot mentioned this pull request Feb 12, 2026
Merged
@coderabbitai coderabbitai bot mentioned this pull request Mar 24, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@bluestreak01 bluestreak01 bluestreak01 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@glasstiger @bluestreak01