Resolve port: CLI > config > default - #29
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a Swedish PortConfigurationGuide.md describing port selection priority and implements a port-resolution flow in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant App
participant Config as "application.yml"
participant TcpServer
User->>CLI: invoke app with args (maybe --port)
CLI->>App: pass args
App->>CLI: parsePortFromCli(args)
alt CLI port provided & valid
App->>App: validatePort(cliPort, "CLI")
App->>TcpServer: start(cliPort)
else CLI absent or invalid
App->>Config: read server.port
App->>App: validatePort(configPort, "config")
App->>TcpServer: start(configPort or default 8080)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@PortConfigurationGuide.md`:
- Around line 1-34: The documentation's code fence after "Exempel:" is left
open; update PortConfigurationGuide.md by adding a short example properties
block showing the port key (e.g., "port=8080" or similar) and then close the
markdown code fence, and optionally reference the DEFAULT_PORT value and
ServerPortResolver in the surrounding text to make the relationship explicit.
🧹 Nitpick comments (2)
src/main/java/org/example/ServerPortResolver.java (1)
28-40: Consider supporting--port=VALUEsyntax.The current implementation only handles
--port VALUE(space-separated). The--port=8080form is also common in CLI tools. This is a minor usability gap.♻️ Optional: add support for `--port=VALUE`
static Integer readPortFromCli(String[] args) { if (args == null) return null; for (int i = 0; i < args.length; i++) { if ("--port".equals(args[i])) { if (i + 1 >= args.length) { throw new IllegalArgumentException("Missing value after --port"); } return parseIntOrThrow(args[i + 1], "Invalid port value after --port"); } + if (args[i].startsWith("--port=")) { + String value = args[i].substring("--port=".length()); + if (value.isEmpty()) { + throw new IllegalArgumentException("Missing value after --port="); + } + return parseIntOrThrow(value, "Invalid port value after --port="); + } } return null; }src/test/java/org/example/ServerConfigTest.java (1)
9-13: Test correctly validates CLI precedence, but coverage is thin.This test effectively proves CLI overrides both config (9090 from
ConnectionConfig.propertieson the test classpath) and default (8080). Consider renaming tocli_port_wins_over_config_and_defaultfor clarity.Additional tests worth adding:
resolvePort(new String[]{})→ should return 9090 (from config file) to verify config > defaultresolvePort(new String[]{"--port"})→ should throwIllegalArgumentExceptionresolvePort(new String[]{"--port", "0"})or"70000"→ should throw for out-of-rangeresolvePort(null)→ should return config port (9090)
JohanHiths
self-requested a review
February 11, 2026 09:46
gitnes94
self-requested a review
February 11, 2026 10:26
|
Ser inga problem, bra kod och test som är tydliga och guide som förklarar pedagogiskt. |
JohanHiths
previously approved these changes
Feb 11, 2026
gitnes94
reviewed
Feb 11, 2026
gitnes94
left a comment
gitnes94
left a comment
There was a problem hiding this comment.
Väldigt tydlig kod, bra förklarad och funkar som den ska!
Bra jobbat!
gitnes94
self-requested a review
February 11, 2026 14:55
gitnes94
previously approved these changes
Feb 11, 2026
viktorlindell12
force-pushed
the
feature/cli-port-override
branch
from
February 16, 2026 12:18
5310b2c to
233d2e2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/main/java/org/example/ServerPortResolver.java (3)
14-26: Consider logging which port source was selected.For operational troubleshooting, it's useful to know which source provided the port (CLI, config file, or default). A simple log statement at each return point in
resolvePortwould help diagnose startup issues.
28-40: CLI parser doesn't handle--port=VALUEformat.The parser only supports
--port VALUE(space-separated). Many CLI tools also accept--port=8080. This isn't a bug per se, but worth noting if users might expect the=syntax.
58-63:DEFAULT_PORTbypasses validation.
resolvePortreturnsDEFAULT_PORTat Line 25 without callingvalidatePort. Currently safe since the constant is8080, but if someone changesDEFAULT_PORTto an invalid value in the future, it would silently be accepted.Defensive fix
- return DEFAULT_PORT; + return validatePort(DEFAULT_PORT, "default");src/test/java/org/example/ServerConfigTest.java (1)
9-13: Test effectively validates CLI > config, but more scenarios are needed.Since
ConnectionConfig.propertieswithport=9090is on the classpath, this test actually proves CLI overrides the config file too — the namecli_port_wins_over_defaultundersells that. Consider adding tests for:
- Config file override (no
--portarg → expect9090from properties)- Default fallback (no CLI, no config file — harder to test with the file on classpath)
- Invalid/out-of-range port values
- Missing value after
--port
viktorlindell12
dismissed stale reviews from JohanHiths and gitnes94
via
February 16, 2026 13:07
954afa5
viktorlindell12
force-pushed
the
feature/cli-port-override
branch
from
February 16, 2026 13:07
233d2e2 to
954afa5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/example/ServerPortResolver.java (1)
28-40: CLI parsing handles edge cases well.Null-safe, bounds-checked, and returns early on first match. One minor note: only the
--port <value>format (space-separated) is supported;--port=<value>would be silently ignored. This is fine if documented, but worth being aware of.Optional: support `--port=VALUE` format
static Integer readPortFromCli(String[] args) { if (args == null) return null; for (int i = 0; i < args.length; i++) { if ("--port".equals(args[i])) { if (i + 1 >= args.length) { throw new IllegalArgumentException("Missing value after --port"); } return parseIntOrThrow(args[i + 1], "Invalid port value after --port"); } + if (args[i].startsWith("--port=")) { + String value = args[i].substring("--port=".length()); + if (value.isEmpty()) { + throw new IllegalArgumentException("Missing value after --port="); + } + return parseIntOrThrow(value, "Invalid port value after --port="); + } } return null; }
Ericthilen
self-requested a review
February 17, 2026 11:27
kappsegla
requested changes
Feb 17, 2026
Closed
viktorlindell12
dismissed stale reviews from apaegs and Ericthilen
via
February 17, 2026 16:11
1b7f1b0
viktorlindell12
force-pushed
the
feature/cli-port-override
branch
from
February 17, 2026 16:11
954afa5 to
1b7f1b0
Compare
apaegs
previously approved these changes
Feb 17, 2026
gitnes94
previously approved these changes
Feb 17, 2026
JohanHiths
previously approved these changes
Feb 18, 2026
viktorlindell12
dismissed stale reviews from JohanHiths, gitnes94, and apaegs
via
February 19, 2026 09:28
0bdd716
apaegs
previously approved these changes
Feb 19, 2026
viktorlindell12
dismissed stale reviews from Ericthilen and apaegs
via
February 19, 2026 15:23
8940634
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/org/example/App.java (1)
12-14:appConfigis unused; the implicit ordering dependency betweenloadOnceandresolvePortis fragile and silent.
ConfigLoader.loadOnce(configPath)is called for its side effect of populating the config cache so thatServerPortResolvercan later callConfigLoader.get()internally. The returnedAppConfigreference is never read, which will trigger a compiler warning. More importantly, if the two calls are ever reordered,resolvePortsilently falls back to the default port (becausegetLoadedConfigOrNullswallows theIllegalStateException), making the config-based port resolution invisible to fail.♻️ Proposed fix — discard the unused return value and document the ordering contract
- AppConfig appConfig = ConfigLoader.loadOnce(configPath); + // Must precede resolvePort so ConfigLoader.get() finds the loaded config. + ConfigLoader.loadOnce(configPath); int port = ServerPortResolver.resolvePort(args);This also allows removing the now-unused
import org.example.config.AppConfig;.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/App.java` around lines 12 - 14, AppConfig returned by ConfigLoader.loadOnce(configPath) is never used and the code relies on a fragile ordering contract with ServerPortResolver.resolvePort(args); update the call to explicitly discard the return value (e.g., call loadOnce for side-effects and don't assign to AppConfig) and add a comment beside ConfigLoader.loadOnce(configPath) documenting that ServerPortResolver.resolvePort(args) depends on the config cache being populated by loadOnce (and that reordering will change behavior); also remove the now-unused AppConfig import and ensure references like ConfigLoader.get() / getLoadedConfigOrNull are left unchanged so the ordering contract is clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PortConfigurationGuide.md`:
- Line 44: Remove the trailing space after the horizontal rule token so the line
reads exactly '---' (no trailing whitespace); locate the offending horizontal
rule (the '--- ' instance in PortConfigurationGuide.md) and trim the trailing
space to satisfy markdownlint MD035.
- Around line 29-31: Replace the plain, malformed inline text "yaml server:
port:9090" with a fenced code block labeled "yaml" containing a valid YAML
snippet where "server" is a mapping and "port" is a nested key set to 9090;
ensure the code fence markers are present so the snippet renders as formatted
YAML and that the YAML structure uses proper indentation (server as a map and
port as its child key).
---
Nitpick comments:
In `@src/main/java/org/example/App.java`:
- Around line 12-14: AppConfig returned by ConfigLoader.loadOnce(configPath) is
never used and the code relies on a fragile ordering contract with
ServerPortResolver.resolvePort(args); update the call to explicitly discard the
return value (e.g., call loadOnce for side-effects and don't assign to
AppConfig) and add a comment beside ConfigLoader.loadOnce(configPath)
documenting that ServerPortResolver.resolvePort(args) depends on the config
cache being populated by loadOnce (and that reordering will change behavior);
also remove the now-unused AppConfig import and ensure references like
ConfigLoader.get() / getLoadedConfigOrNull are left unchanged so the ordering
contract is clear.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@PortConfigurationGuide.md`:
- Around line 29-32: Wrap the example in a fenced YAML block and fix indentation
so `server` is a map with `port` nested under it; replace the plain text snippet
with a triple-backtick fenced block labeled yaml and ensure `server:` is on its
own line and `port:` is indented (e.g., two spaces) under it with the value
`9090`, so the `server` and `port` keys form a valid YAML mapping.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PortConfigurationGuide.md`:
- Around line 11-38: Rename and reorder the sections so the documented priority
matches the intro list and includes the missing CLI tier: add a new "CLI"
section as the highest priority, then "Config-fil: `application.yml`", then
"Default-värde", and finally "Sammanfattning"; ensure all four headings use the
same level (make "Default-värde", "Config-fil: `application.yml`", "CLI", and
"Sammanfattning" all H2, e.g., `##`) and update the opening priority list to
read "1) CLI, 2) Config-fil, 3) Default, 4) Sammanfattning" so order and
numbering are consistent throughout the document.
- Around line 1-8: Update the document title and priority list to include the
CLI override as the highest-priority input: add a step 1 "CLI flag (`--port`) —
overrides config and default", shift the existing "Config-fil (`application.yml`
: `server.port`)" to step 2 and "Default (`8080`)" to step 3, and mention that
`--port` takes precedence over `server.port` and the default; also remove the
stray leading hyphen before the en-dash on the default line.
---
Duplicate comments:
In `@PortConfigurationGuide.md`:
- Around line 26-29: The YAML example is plain text and invalid; wrap the
example in a fenced code block and correct its structure so `port` is nested
under `server` (e.g., add triple backticks before and after the block and
replace the current `yaml server: port:9090` with a properly indented two-line
YAML snippet where `server:` is a parent and `port: 9090` is indented beneath
it); ensure the fenced block specifies the language tag `yaml` for proper
rendering.
- Line 4: Remove the stray trailing space on the blank line near the top of the
document (it should be a truly empty line to satisfy MD009) and re-save the
file; also scan each horizontal rule line that uses '---' and strip any trailing
spaces there to address the possible MD035 issue so all '---' lines have no
trailing whitespace.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/example/App.java (1)
30-47:--port=valueform is silently ignored.The parser only recognises the space-separated form
--port 8000. If a user passes--port=8000as a single token, thePORT_FLAG.equals(args[i])check never matches and the method returnsnull, silently falling back to the config port without any warning. This is a minor but easy-to-hit UX gap.♻️ Handle the `--port=` form
for (int i = 0; i < args.length; i++) { if (PORT_FLAG.equals(args[i])) { int valueIndex = i + 1; if (valueIndex >= args.length) { throw new IllegalArgumentException("Missing value after " + PORT_FLAG); } try { return Integer.parseInt(args[valueIndex]); } catch (NumberFormatException e) { throw new IllegalArgumentException("Invalid port value after " + PORT_FLAG + ": " + args[valueIndex], e); } + } else if (args[i].startsWith(PORT_FLAG + "=")) { + String value = args[i].substring(PORT_FLAG.length() + 1); + if (value.isEmpty()) { + throw new IllegalArgumentException("Missing value after " + PORT_FLAG + "="); + } + try { + return Integer.parseInt(value); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("Invalid port value in " + args[i], e); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/App.java` around lines 30 - 47, The CLI port parser parsePortFromCli currently only matches exact PORT_FLAG tokens and ignores the single-token form "--port=8000"; update parsePortFromCli to also detect tokens that start with PORT_FLAG + "=" (e.g., args[i].startsWith(PORT_FLAG + "=")), extract the substring after the '=' as the value, validate that the value is present (throw IllegalArgumentException if empty), parse it with Integer.parseInt and throw the same IllegalArgumentException with the caught NumberFormatException as the cause (same behavior as the space-separated branch), while keeping the existing space-separated handling for PORT_FLAG when encountered as a separate token.src/test/java/org/example/AppPortResolutionTest.java (1)
8-20: Recommend covering the exception-throwing edge cases.Both tests exercise the happy path only. The three
IllegalArgumentException-throwing branches inparsePortFromCliandvalidatePorthave no coverage:
Scenario Method under test nullargs → falls back to configPortresolvePort--portwith no following valueparsePortFromCli--port abc(non-numeric)parsePortFromCli--port 0/--port 65536(out of range)validatePortconfigPort out of range (e.g., 0)validatePort♻️ Proposed additional tests
+import static org.assertj.core.api.Assertions.assertThatThrownBy; + class AppPortResolutionTest { `@Test` void cli_port_wins_over_config() { … } `@Test` void config_port_used_when_no_cli_arg() { … } + + `@Test` + void null_args_falls_back_to_config_port() { + int port = App.resolvePort(null, 9090); + assertThat(port).isEqualTo(9090); + } + + `@Test` + void missing_value_after_port_flag_throws() { + assertThatThrownBy(() -> App.resolvePort(new String[]{"--port"}, 9090)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Missing value"); + } + + `@Test` + void non_numeric_port_throws() { + assertThatThrownBy(() -> App.resolvePort(new String[]{"--port", "abc"}, 9090)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Invalid port value"); + } + + `@Test` + void cli_port_out_of_range_throws() { + assertThatThrownBy(() -> App.resolvePort(new String[]{"--port", "0"}, 9090)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("out of range"); + } + + `@Test` + void config_port_out_of_range_throws() { + assertThatThrownBy(() -> App.resolvePort(new String[]{}, 0)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("out of range"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/AppPortResolutionTest.java` around lines 8 - 20, Add unit tests to cover the exception-throwing branches of App.resolvePort/parsePortFromCli/validatePort: write tests that assert IllegalArgumentException is thrown when (1) resolvePort is called with null args and an invalid configPort, (2) parsePortFromCli is given args containing "--port" with no following value, (3) parsePortFromCli is given "--port abc" (non-numeric), (4) validatePort rejects out-of-range CLI values like "0" and "65536", and (5) validatePort rejects an out-of-range configPort (e.g., 0); use the existing test class AppPortResolutionTest and assert exceptions for each scenario so all error branches are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/example/App.java`:
- Around line 30-47: The CLI port parser parsePortFromCli currently only matches
exact PORT_FLAG tokens and ignores the single-token form "--port=8000"; update
parsePortFromCli to also detect tokens that start with PORT_FLAG + "=" (e.g.,
args[i].startsWith(PORT_FLAG + "=")), extract the substring after the '=' as the
value, validate that the value is present (throw IllegalArgumentException if
empty), parse it with Integer.parseInt and throw the same
IllegalArgumentException with the caught NumberFormatException as the cause
(same behavior as the space-separated branch), while keeping the existing
space-separated handling for PORT_FLAG when encountered as a separate token.
In `@src/test/java/org/example/AppPortResolutionTest.java`:
- Around line 8-20: Add unit tests to cover the exception-throwing branches of
App.resolvePort/parsePortFromCli/validatePort: write tests that assert
IllegalArgumentException is thrown when (1) resolvePort is called with null args
and an invalid configPort, (2) parsePortFromCli is given args containing
"--port" with no following value, (3) parsePortFromCli is given "--port abc"
(non-numeric), (4) validatePort rejects out-of-range CLI values like "0" and
"65536", and (5) validatePort rejects an out-of-range configPort (e.g., 0); use
the existing test class AppPortResolutionTest and assert exceptions for each
scenario so all error branches are covered.
kappsegla
approved these changes
Feb 22, 2026
JohanHiths
approved these changes
Feb 23, 2026
Boppler12
pushed a commit
that referenced
this pull request
Feb 25, 2026
* Resolve port: CLI > config > default * Wire port resolution to AppConfig/ConfigLoader and update docs/tests * Update PortConfigurationGuide.md * Update PortConfigurationGuide.md * Remove ServerPortResolver; use ConfigLoader for port * Update PortConfigurationGuide.md * Update PortConfigurationGuide.md * may be done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CLI (
--port) overrides config (ConnectionConfig.properties) and default port (8080).Includes unit test for CLI priority.
Summary by CodeRabbit