Skip to content

Commit

Permalink
Improve PortAssigner#assignDynamicPorts javadocs
Browse files Browse the repository at this point in the history
* Add more details to the javadoc to better explain the
  differences in behavior when using HTTP vs HTTPS ports.
* Emphasize the word 'replace' in a debugging log message.
* Make the TestConfig nested class public inside
  ConfigResourceTest, because IntelliJ flagged this as
  a case where the class using it (TestApp) was public
  and had more visibility than a class it was using.
  While this didn't cause the test any problems, better
  to listen to IntelliJ when it tells you something!
  And it's correct that normally, this would have
  been an issue in production code.

Closes #428
  • Loading branch information
sleberknight committed Jan 17, 2024
1 parent 205e2da commit af7e7fb
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,22 @@ private PortAssigner(LocalPortChecker localPortChecker,
}

/**
* Sets up the connectors with a dynamic available port if {@link PortAssigner} is configured for dynamic ports.
*
* @implNote When using {@link PortSecurity#SECURE}, the current implementation <strong>replaces</strong> the
* Sets up the application and admin connectors with a dynamic available port
* if {@link PortAssigner} is configured for dynamic ports.
* <p>
* When using {@link PortSecurity#NON_SECURE HTTP},
* only the <em>first</em> application and admin connectors have dynamic ports assigned.
* If an application is using multiple connectors, it's best to avoid using this class
* unless you only want the first ones dynamically assigned.
* <p>
* When using {@link PortSecurity#SECURE HTTPS}, the current implementation <strong>replaces</strong> the
* application and admin connector factories, which overrides any explicit configuration.
* Support for explicit configuration of secure connectors while still assigning dynamics ports may
* be implemented in the future if custom configuration is needed.
* <p>
* <strong>WARNING</strong>: If you need to change specific properties of secure ports,
* or need more than one secure application and/or admin port, you can't use this
* class, since it will replace all other ports!
*/
public void assignDynamicPorts() {
if (portAssignment == PortAssignment.STATIC) {
Expand All @@ -94,7 +104,7 @@ public void assignDynamicPorts() {
}

private void assignSecureDynamicPorts() {
LOG.debug("Secure (https): replace Dropwizard HTTP app/admin connectors with HTTPS ones using dynamic ports");
LOG.debug("Secure (https): *replace* Dropwizard HTTP app/admin connectors with HTTPS ones using dynamic ports");

var usedPorts = new HashSet<Integer>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class ConfigResourceTest {
@Getter
@Setter
@AllArgsConstructor
static class TestConfig extends Configuration {
public static class TestConfig extends Configuration {
private String someNeededProperty;
private String someHiddenPassword;
private CacheConfig cacheConfig;
Expand Down

0 comments on commit af7e7fb

Please sign in to comment.