Skip to content

Use port 443 by default when TLS is enabled and no port is specified - #1339

Merged
ikhoon merged 1 commit into
line:mainfrom
ikhoon:default-tls-port
Jul 24, 2026
Merged

Use port 443 by default when TLS is enabled and no port is specified#1339
ikhoon merged 1 commit into
line:mainfrom
ikhoon:default-tls-port

Conversation

@ikhoon

@ikhoon ikhoon commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Motivation:

AbstractCentralDogmaBuilder.host(String) always assumed the default port 36462 even when useTls() was enabled, so a client configured with TLS and no explicit port silently sent HTTPS requests to the cleartext port instead of 443.

Modifications:

  • Deferred default port resolution from host(String) to hosts() so that the result no longer depends on the order of host() and useTls() calls. A host added without a port is stored with a sentinel port and resolved to 443 if TLS is enabled, or 36462 otherwise.
  • Documented the new default port in the Javadoc and the Java client tutorial.

Result:

  • A CentralDogma client built with useTls() and host(host) without a port number now connects to port 443 instead of 36462.
  • Behavioral change: users who serve TLS on port 36462 (or any non-443 port) should specify the port explicitly, e.g. .host("example.com", 36462).

Motivation:

`AbstractCentralDogmaBuilder.host(String)` always assumed the default
port 36462 even when `useTls()` was enabled, so a client configured with
TLS and no explicit port silently sent HTTPS requests to the cleartext
port instead of 443.

Modifications:

- Defer default port resolution from `host(String)` to `hosts()` so that
  the result no longer depends on the order of `host()` and `useTls()`
  calls. A host added without a port is stored with a sentinel port and
  resolved to 443 if TLS is enabled, or 36462 otherwise.
- Document the new default port in the Javadoc of `host(String)`,
  `useTls()` and `hosts()`, and in the Java client tutorial.

Result:

- A `CentralDogma` client built with `useTls()` and `host(host)` without
  a port number now connects to port 443 instead of 36462. Users who
  relied on serving TLS on port 36462 should specify the port explicitly.
@ikhoon
ikhoon requested review from jrhee17 and minwoox as code owners July 22, 2026 13:04
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The builder now defers default port selection for hosts without explicit ports until host resolution, selecting TLS port 443 when enabled. Tests cover host and URI cases, and documentation describes the TLS default.

Changes

TLS port resolution

Layer / File(s) Summary
Deferred host port resolution
client/java/src/main/java/.../AbstractCentralDogmaBuilder.java, site/src/sphinx/client-java.rst
Unspecified host ports are stored with a sentinel and resolved to the cleartext or TLS default when hosts() is called; documentation notes the TLS default of port 443.
Port resolution validation
client/java/src/test/java/.../CentralDogmaBuilderTest.java, client/java-armeria/src/test/java/.../ArmeriaCentralDogmaBuilderTest.java
Tests verify TLS and cleartext defaults, call ordering, explicit-port retention, deduplication, URI schemes, IP literals, and Armeria endpoint resolution.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: jrhee17, minwoox

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately summarizes the main behavioral change: defaulting to port 443 for TLS when no port is specified.
Description check ✅ Passed The description matches the implemented port-resolution change and accompanying documentation updates.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@ikhoon ikhoon added the defect label Jul 22, 2026
@ikhoon ikhoon added this to the 0.86.0 milestone Jul 22, 2026

@jrhee17 jrhee17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 👍

@minwoox minwoox left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@ikhoon
ikhoon merged commit 852e082 into line:main Jul 24, 2026
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants