Skip to content

Conversation

amotl
Copy link
Member

@amotl amotl commented Oct 8, 2025

@amotl amotl marked this pull request as ready for review October 8, 2025 01:25
Copy link

coderabbitai bot commented Oct 8, 2025

Walkthrough

Replaced the Java connection docs with a structured page that includes link includes, separate PostgreSQL JDBC and CrateDB JDBC sections (Synopsis, Maven, Gradle, Download), examples and a full CrateDB JDBC runnable example, plus updated testing references. Documentation-only changes; no code/API edits. (≈ thirty words)

Changes

Cohort / File(s) Summary of changes
Java connection docs
docs/connect/java.md
Rewrote and restructured the Java connection doc: added include for links, introduced PostgreSQL JDBC and CrateDB JDBC sections (Synopsis, Maven, Gradle, Download cards), added properties-based and JDBC-URL examples, a full runnable CrateDB JDBC example, example/play cards, testing links (java-junit, testcontainers), and formatting/indentation tweaks.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

maintenance

I twitch my whiskers, hop and write,
JDBC pages snug tonight.
Crate and Postgres side by side,
Maven notes and examples tied. 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title “Java: Modernize page” directly references the core update of the pull request, which is to modernize the Java/JDBC documentation page, and does so in a concise, clear manner without unnecessary detail or ambiguity. It succinctly conveys the primary intent of the changeset, making it easy for teammates to understand the focus at a glance. The phrasing avoids generic terms like “misc” or “updates” and aligns with the documented modifications.
Description Check ✅ Passed The description clearly states the intent to modernize the Java/JDBC page, provides a preview link to review the changes, and includes relevant issue references, all of which align with the actual modifications in the pull request. It stays on topic by summarizing what was updated and where to see it, without introducing unrelated information.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch java

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3a3cbc4 and bb36551.

📒 Files selected for processing (1)
  • docs/connect/java.md (3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-08T01:34:18.867Z
Learnt from: amotl
PR: crate/cratedb-guide#385
File: docs/connect/java.md:48-51
Timestamp: 2025-10-08T01:34:18.867Z
Learning: CrateDB JDBC driver uses the `jdbc:crate://` protocol scheme but communicates via the PostgreSQL wire protocol on port 5432, just like the PostgreSQL JDBC driver (`jdbc:postgresql://`). Do not confuse the `jdbc:crate://` scheme with other protocol schemes like `crate://` (used by SQLAlchemy dialect for CrateDB) or assume it uses the HTTP endpoint on port 4200.

Applied to files:

  • docs/connect/java.md
📚 Learning: 2025-08-05T07:14:57.416Z
Learnt from: hammerhead
PR: crate/cratedb-guide#221
File: docs/connect/configure.md:58-66
Timestamp: 2025-08-05T07:14:57.416Z
Learning: In CrateDB connection strings, the user:password@ syntax is valid for HTTP Basic authentication on port 4200, but PostgreSQL JDBC drivers do not support this format and require credentials as query parameters (?user=<user>&password=<password>) instead.

Applied to files:

  • docs/connect/java.md
🪛 markdownlint-cli2 (0.18.1)
docs/connect/java.md

84-84: Bare URL used

(MD034, no-bare-urls)


135-135: Bare URL used

(MD034, no-bare-urls)


178-178: Bare URL used

(MD034, no-bare-urls)

⏰ 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). (1)
  • GitHub Check: Build docs

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.

@amotl amotl added sanding-1200 Fine sanding. cross linking Linking to different locations of the documentation. new content New content being added. labels Oct 8, 2025
Copy link

@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: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 94dfc73 and ff3709c.

📒 Files selected for processing (1)
  • docs/connect/java.md (3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-05T07:14:57.416Z
Learnt from: hammerhead
PR: crate/cratedb-guide#221
File: docs/connect/configure.md:58-66
Timestamp: 2025-08-05T07:14:57.416Z
Learning: In CrateDB connection strings, the user:password@ syntax is valid for HTTP Basic authentication on port 4200, but PostgreSQL JDBC drivers do not support this format and require credentials as query parameters (?user=<user>&password=<password>) instead.

Applied to files:

  • docs/connect/java.md
🪛 markdownlint-cli2 (0.18.1)
docs/connect/java.md

83-83: Bare URL used

(MD034, no-bare-urls)


167-167: Bare URL used

(MD034, no-bare-urls)

⏰ 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). (1)
  • GitHub Check: Build docs

Comment on lines +46 to +99
properties.put("password", "<PASSWORD>");
properties.put("ssl", true);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Store JDBC properties as strings

Properties.getProperty(...) returns only String values. By writing a Boolean (true) for ssl, the driver sees null, so SSL stays off. Use setProperty("ssl", "true") (and consider doing the same for the other keys for consistency).

-properties.put("ssl", true);
+properties.setProperty("ssl", "true");

Apply this in both JDBC snippets and the full example.

Also applies to: 102-104, 132-134

🤖 Prompt for AI Agents
In docs/connect/java.md around lines 46-47 (and also apply the same change at
lines 102-104 and 132-134), JDBC Properties are being set with a boolean value
for "ssl" which causes Properties.getProperty(...) to return null; change these
to use string values via Properties.setProperty("ssl", "true") (and similarly
setProperty for other keys like "user" and "password" to keep consistency) in
both small JDBC snippets and the full example so the driver reads the values
correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

@matriv: Do you agree with this evaluation, or is it nonsense?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://jdbc.postgresql.org/documentation/use

ssl (boolean) Default false
Connect using SSL. The server must have been compiled with SSL support. This property does not need a value associated with it. The mere presence of it specifies an SSL connection. However, for compatibility with future versions, the value “true” is preferred. For more information see [Using SSL](https://jdbc.postgresql.org/documentation/ssl/).
Setting up the certificates and keys for ssl connection can be tricky see [The test documentation](https://github.com/pgjdbc/pgjdbc/blob/master/certdir/README.md) for detailed examples.

but in their example above:

props.setProperty("ssl", "true");

So I guess both work

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. So, we'll keep this unmodified, right?

@amotl amotl requested review from matriv and seut and removed request for matriv and seut October 8, 2025 01:36
Copy link
Member Author

Choose a reason for hiding this comment

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

This comment was marked as off-topic.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend not to, I think maybe only the testing fits here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a note per c2a090d, thanks.

@amotl amotl requested review from matriv and seut October 8, 2025 02:21
Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

looks good, thx, just one comment.

while CrateDB JDBC uses `jdbc:crate://`.
:::

Applications using the PostgreSQL JDBC Driver may emit PostgreSQL-specific
Copy link
Contributor

Choose a reason for hiding this comment

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

imho we should rephrase to encourage using the postgres jdbc and only if they encounter problems turn to our crate one.

Copy link
Member Author

@amotl amotl Oct 8, 2025

Choose a reason for hiding this comment

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

I am still not convinced we should really encourage people to use the vanilla driver, because the CrateDB driver offers so many excellent usability features?

I think the shortcomings of the vanilla driver are quickly present, so all serious Java applications are using the CrateDB driver, if I am not mistaken, like our Flink demo code is also doing it? Could you provide feedback here, @zolbatar and @mackerl?

Of course I might not be up-to-date about the situation, so please correct me if I am wrong. Nevertheless, I've added 9f757e3 at your disposal; please suggest any wording improvements. 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK Postgres driver is the first option, this is the one we use in our java integration tests in crate/crate repo.
This is the one that we use for crate-qa client tests. CrateDB is to be used only when hiccups are encountered with the stock jdbc driver.

So I think we should also interchange the sections and mention first the PostgreSQL jdbc and then ours.

Copy link
Member Author

@amotl amotl Oct 9, 2025

Choose a reason for hiding this comment

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

Thanks. I've swapped the sections, know about your rationale, but I am still challenging it strongly. I think we are doing a bad service to CrateDB if we keep it like this, because subtle differences in databases should actively be balanced within the driver layer, to not let application developers stranded. All the competitors are doing it excellently for both JDBC and ODBC, and I would like to see that CrateDB will start doing the same.

For example, if we don't modernize and maintain the CrateDB JDBC Driver, we will never be able to conceive a Hibernate dialect on top of it, and only then any database will start getting interesting for many.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't argue against that, but the situation currently is that stock JDBC is in favor.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not about this PR, so let's merge it. Still I am interested how this situation came into existence. As long as we keep it that way, including the thinking perspective, I fear not much will change here. Maybe there will be another discussion about this topic at a better spot soon. Please provide relevant guidance when possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

The decision back in the past is that we don't have the manpower to keep track of psql jdbc changes and incorporate them to our own version of the driver and to rather try to make our CrateDB (server side) compatible with the new changes in postgres jdbc, which also makes sense to make it more and more compatible with PostgreSQL tooling in general.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @matriv that the Postgres driver is the first option.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend not to, I think maybe only the testing fits here.

coderabbitai[bot]

This comment was marked as resolved.

Copy link
Contributor

@matriv matriv left a comment

Choose a reason for hiding this comment

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

LGTM, thx!
Added one more comment though about the ordering: #385 (comment)

@amotl amotl merged commit 79cbb6e into main Oct 9, 2025
3 checks passed
@amotl amotl deleted the java branch October 9, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cross linking Linking to different locations of the documentation. new content New content being added. sanding-1200 Fine sanding.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants