Light code review #1

[mrwilson]

Hi all, at the request of @jamietanna I've done a quick look-over and some small things jumped out. None of them are game-breakers and you'd be reasonably justified in not acting on most of these.

Records

I notice you've got an issue for this already (#4). Records are a good replacement for data classes as immutable containers, but do not support setters out-of-the-box and I don't know about its compatibility with Spring auto-serialisation.

If both of these things are fine for your use case, then the data classes Data and ApiMetadata would become something like public record Data(String apiVersion, Data data) {}

I notice in #5 that there are data access objects that might be in play soon; even if you keep the original classes as non-records, they would be a great fit for read-only data because of their immutability. In this case, fromDao()/toDao() methods might be useful to take that boilerplate out of call-site code.

Tests

There's a jsonContent field in ApiMetadataTest. You could probably remove it in this class and have local variables in each test because (as far as I can tell) this data is not shared.

Even though this project uses AssertJ and I'm more familiar with Hamcrest, I would extol the virtues of custom matchers/assertions. If it's an object that's going to be in lots of tests, then this will pay off in readability quite quickly.

// Before
assertThat(apiMetadata.getApiVersion()).isEqualTo(apiVersion);

// After
assertApi(apiMetadata).hasVersion(apiVersion);

var

I'm a big fan of using var for readability where the type can be reasonably inferred from the context. For example:

// Before
BulkApiMetadataResponse bulkApiMetadataResponse = new BulkApiMetadataResponse();

// After
var response = new BulkApiMetadataResponse();

Gradle configuration

I'd advocate turning on reproducible builds in the gradle config. It costs basically nothing, and it's one of the things for the highest level of Supply-chain Levels for Software Artifacts (SLSA).

[nlsteers]

+1 for var.

For projects not on Java14, Lombok can do a lot of heavy lifting re boiler plate on data classes.

[jamietanna]

😬 I'd rather use IntelliJ / manually hand write getters and setters than Lombok

[nlsteers]

Why so? Saves a lot of time / tedium!

[jamietanna]

I've worked on a few Lombok projects in the past - before we ripped it out - and it was generally:

• developers need i.e. the Lombok IntelliJ plugin to be able to get up and running
• a lot of "magic" behind the scenes that slows onboarding
• as soon as you introduce Lombok as a dependency we then open up the ability to do things like @SneakyThrows which is risky

If you're not using an IDE that can generate for you all the methods, then yeah I get that, but personally, it doesn't give me anything beneficial

[nlsteers]

Appreciate your insight.

That being said if you don't want @SneakyThrows in your project it should be part of project coding standards and addressed in peer review rather than heavy handed restrictions on QOL deps.

Re your first point, you don't need the plugin if your build tool is set up correctly but it's well taken as without it your syntax linter will red line everything.

IMHO, you can have the best of both worlds. If behind the scenes 'magic' is a problem you might take umbrage with Spring Boot 😝.

e: you could have something like lombok.sneakythrows.flagUsage = error in your lombok.config