kiwiproject / registry-aware-jersey-client Goto Github PK

It does not make sense to allow RegistryAwareClient to be constructed with a null RegistryClient, since that results in a NPE whenever a targetForService method is called.

RegistryAwareClientBuilder provides the headersSupplier builder method, which provides a way to specify that the given "header supplier" should be used to add request headers to all client requests.

We should add this same ability in the DropwizardManagedClientBuilder.

The incorrect log message is:

if (!sslContextWasSetOnThis) {
    LOG.info("No SSLContext provided; this client will use system default TLS via SSLConnectionSocketFactory.getSocketFactory()");
}

If an SSLContext is not set, then one is not used.

Alternatively, we could actually do what the log message says instead, but is that a good default?

Currently in DropwizardManagedClientBuilder#newDefaultJerseyClientConfiguration:

If the caller supplies a non-null TlsConfigProvider...

and that provider's canProvide() returns false...

we skip setting the TlsConfiguration on the JerseyClientConfiguration...

but we don't provide any feedback on this situation...

which is presumably not what the caller intended.

This task is to simply log at WARN level the fact that:

1. A non-null TlsConfigProvider was supplied, but...
2. It cannot provide TLS configuration values...
3. So we are unable to set the TLS configuration on the JerseyClientConfiguration

kiwi-parent 2.0.0 removed slf4j-api and logback-classic and they are now controlled only in the BOM (kiwi-bom). So, they need to be explicitly added back to the POM, but don't need version specified. slf4j-api should be compile scope (default) and logback-classic should be test scope.

For example:

serviceId.forAdminPort()

Or maybe use "with":

serviceId.withAdminPort()

serviceId.withApplicationPort()

Add adapter methods to ServiceIdentifier to convert the Dropwizard Duration to millis as an int. This is needed to ensure the values don't overflow since Jersey timeouts "MUST be an instance convertible to Integer".

See the javadocs for CONNECT_TIMEOUT and READ_TIMEOUT

We want to move away from JSR 305 due to "split packages".

References:

• Making JSR 305 Work On Java 9
• Java 9 Migration Guide: The Seven Most Common Challenges
• The Java Module System (book by Nicolai Parlog)
• Dropwizard: maven-javadoc-plugin fails due to Automatic-Module-Name
• Dropwizard: Migrate away from JSR305

This issue came from this discussion: #47

Create new overloaded method in RegistryAwareClient:

targetForService(String serviceName, PortType portType)

Most of the time, in our (Dropwizard) configuration classes, we simply use

@NotNull
@Value
private ServiceIdentifier myServiceId = ServiceIdentifier.builder().serviceName("my-service").build();

which is rather verbose. Instead, we should provide a simple factory method to permit something like this:

@NotNull
@Value
private ServiceIdentifier myServiceId = ServiceIdentifier.ofName("my-service");

Not sure whether ofName should be the name, or maybe application to indicate it creates an instance with an APPLICATION connector type. ServiceIdentifier.application("my-service") doesn't seem as clear, though ofName doesn't indicate the connector type. Or maybe just ServiceIdentifier.of("my-service") which would allow for also having ServiceIdentifier.of("my-service", PortType.ADMIN) if we wanted.

Consider moving to a top-level class.

Also consider moving the static factory method newMissingServiceRuntimeException into the exception. Might want to rename it, e.g.

throw new MissingServiceRuntimeException.from(serviceIdentifier);

DropwizardManagedClientBuilder has a private newJerseyClientConfiguration that sort of fits the bill. Looks like we could add two methods:

• public static newJerseyClientConfiguration() - this one will not add a TlsConfiguration
• public static newJerseyClientConfiguration(TlsConfigProvider) - this will add a TlsConfiguration

Currently when we release retrying-again, kiwi, and kiwi-test it forces us to release all our other libraries in kiwiproject. As long as the updates to these pure utility libraries are 100% backwards compatible, then it should be safe to update them without having to update every other library.

This task is to add a dependencyManagement section to the POM to control the version of these libraries and avoid all the downstream convergence errors. It will allow us to release them on a more frequent cadence, since they are the ones that change more frequently.

Add the timeoutsFrom method to DropwizardManagedClientBuilder. This was mentioned in this comment in discussion #47, but I did not actually do that part.

The specific test that starts failing is DropwizardClientsTest.AddJacksonMessageBodyProvider#shouldDeserializeJsonResponses

Once on Jackson 2.15.x, change the code in the newSamplePerson test helper method which sets the createdAt and updatedAt properties from:

ZoneId.of("UTC")

to

ZoneId.of("UTC").normalized()

We could also use ZoneOffset.UTC but I think the normalized() method makes it a bit more clear, especially once you go read its javadoc.

We can also extract ZoneId.of("UTC").normalized() to a local variable instead of repeating ourselves, e.g.

var utc = ZoneId.of("UTC").normalized();
// ...
    .createdAt(ZonedDateTime.of(..., utc))
    .updatedAt(ZonedDateTime.of(..., utc))
// ...

See here for more details: kiwiproject/kiwi-bom#655 (comment)

This workflow will allow us to easily Delombok the code and view the resulting source code.

This will help when diagnosing CodeQL problems since it references line numbers in Delomboked source files.

The workflow should be manually triggered.

If the repository is not up to date, print a warning and exit the release script.

We can copy a ServiceIdentifier instance using:

var copy = serviceId.toBuilder().build();

This is most useful when you want to change something, e.g.

var newServiceId = serviceId.toBuilder()
    .preferredVersion("2020.12.01")
    .build();

But when all we want is an exact copy and not to change anything it would be nice to be able to do:

var copy = ServiceIdentifier.copyOf(serviceId);

This will add the following:

1. ClientBuilder (interface)
2. RegistryAwareClientBuilder (impl)
3. ClientBuilders (factory)

We don't want to add a hard dependency to Dropwizard in ClientBuilders, ClientBuilder, or RegistryAwareClientBuilder, so we will create a separate utility class in the dropwizard package.

We currently have a targetForService which sets up the WebTarget using the service instance's hostname, application port and home page url. It would be nice to have at least the following option to request data from the admin port:

public WebTarget adminTargetForService(ServiceIdentifier serviceIdentifier)

This will setup the WebTarget with the hostname and admin port but then let the caller set the path. We could also add:

public WebTarget adminStatusTargetForService(ServiceIdentifier serviceIdentifier) and public WebTarget adminHealthCheckTargetForService(ServiceIdentifier serviceIdentifier) that will utilize the other path objects in the ServiceInstance to set the base target.

Ability to hook the client into dropwizard's managed process