Added Base64 implementation to simpleclient_common to remove jax-api …

[dhoard]

Added Base64 implementation to simpleclient_common to remove jax-api dependency. Abstracted Base64 implementation to be more Java 8 like/make it easier to replace if required.

[dhoard]

@fstab please review.

[fstab]

The code is looking good, however, I would like to move it out of the simpleclient_common module. That module is used by a lot of other projects and I would like to keep it's scope as small as possible.

As far as I can see the only usage in production code is in simpleclient_pushgateway. Could you move it there?

The other usage is in a test in simpleclient_httpserver, but I guess it does not harm to keep javax.xml.bind as a test dependency in that module.

[dhoard]

@fstab Thanks for the review! My thoughts around putting it into simpleclient_common...

1. I am hoping we can get HTTP authentication in jmx_exporter soon, which would require a Base64 implementation for Java 6/7 compatibility.

2. Other users of client_java may also want to implement HTTP authentication when using HTTPServer and therefore also need it for Java 6/7 compatibility.

By putting into simpleclient_pushgateway we essential push the issue to downstream projects to solve the Base64 issue to maintain Java 6/7 compatibility when using simpleclient_httpserver. A common implementation seems like it could be beneficial.

I'm concerned that if/when we add HTTP authentication to jmx_exporter this will be deemed something that needs to be addressed by client_java/simple_httpserver which means either keeping two versions of the Base64 code in client_java... one in simpleclient_pushgateway and one in simpleclient_httpserver or refactoring back to a common implementation in simpleclient_common.

Thoughts?

[wilkinsona]

Instead of maintaining an implementation of Base64 encoding, I wonder if it would be better to use java.util.Base64 when it's available and fall back to javax.xml.bind.DatatypeConverter when it is not?

[dhoard]

@wilkinsona I thought of that approach, but not sure it would solve the issue. The goal is not to have a dependency on JAX APIs whatsoever.

For Java 8+, it would be easy, we could use the Java version and use java.util.Base64 via delegation/reflection (Not sure we want to introduce reflection in the code path.)

For Java 6/7, there is nothing to fall back to unless we include a Base64 implementation.

Am I missing something?

[wilkinsona]

DatatypeConverter is part of the JDK in Java 6 (and 7 and 8). It moved into a separate module in Java 9 and was removed entirely in Java 11. For Java 6 and 7, you can delegate to DatatypeConverter. For Java 8+, you can delegate to java.util.Base64. As long as you can compile against Java 8 while producing Java 6-compatible byte code, you can do so without reflection.

[dhoard]

Good points. I'll see what I can do when I get some time.

[dhoard]

@wilkinsona I have implemented the code as you described. Works as expected.

@fstab I still feel this should be in client_java/simpleclient_common since it will be a benefit to both client_java and any projects dependent on client_java. (I have moved it back in the latest PR for review)

[fstab]

Thanks a lot for the discussion! I thought about this, and I am hesitant to add a dependency or a Base64 implementation to simpleclient_common.

simpleclient_common is used way beyond the client_java ecosystem: It's in Micrometer, which ships with Spring Boot, Quarkus, and others. And it's also part of the Java implementation of the upcoming OpenTelemetry metrics standard.

That means lots of applications out there have simpleclient_common as an indirect dependency, even if they aren't aware and don't use the client_java library. We should be very conservative with what we add to that module. What if somebody implements an incompatible jaxb-api version 3 in the future?

By putting into simpleclient_pushgateway we essential push the issue to downstream projects to solve the Base64 issue to maintain Java 6/7 compatibility when using simpleclient_httpserver.

This is correct, but as 3rd party projects using client_java are unlikely to support Java 6 they can just use java.util.Base64 and won't have an issue.

The main project where this is relevant is the jmx_exporter, but given how widely simpleclient_common is used I'd rather push the issue downstream to jmx_exporter and risk a bit of copy-and-paste between simpleclient_pushgateway and the jmx_exporter than putting a Base64 implementation in simpleclient_common.

[dhoard]

@fstab @wilkinsona changes made. please review.

[fstab]

Thanks a lot! I added two more nitpicks, apart from that it's ready to be rebased, squashed and merged 🎉.

[fstab]

Thanks a lot! I can confirm that it works with Zulu's Java 6 as well as OpenJDK 11 and 17.