Support JPMS, edit README, idiomize tests, shared ObjectMapper, java.time usage…

[Mechite]

Copy-paste of commit message:

This is a large commit which deprecates a lot of things and changes the API. Features it brings includes asynchronous connections (returning CompletableFutures on request), a total lack of runtime reflections (no use of Jackson ObjectMapper, etc, and instead compile-time processing to continue to maintain a clean codebase), all suitable POJOs implementing Comparable and Serializable (Comparable isn't a debatable one, but Serializable is useful if you want to send a Gorse request result over an internal work queue, etc, useful for reactive code if you want to use the result elsewhere), and much more idiomatic code (a lot of fields, accessors etc were not very idiomatic due to Gorse obviously being written in Go). It also bumps the version of dependencies and idomizes the tests too (using assertThrows, etc). How much of this proposal is expected to be accepted is a varied answer, as while a lot of the changes and improvements are very beneficial, there is an increased number of dependencies as Avaje Jsonb and Avaje HTTP Client are used in order to simplify the codebase and achieve these benefits. The original JDK HttpClient usage that was used before could be reverted to while keeping the Jsonb usage to achieve the reflection-free approach, Retrofit used instead of Avaje, etc. There are many options for how you can go about merging this commit.

All of the code here has NOT BEEN TESTED! The unit tests required a local instance of Gorse running, it was not mocked, so this has been contributed blindly. While I did use TestContainers to implement a whole mocking system for others to test their Gorse code with, as well as for GorseTest to use instead of connecting to a server that you have to host in order to build Gorse with the tests enabled, the main issue is that TestContainers has no JPMS support so the project won't build with it. Once this has been fixed (testcontainers/testcontainers-java#7337) this is safe to merge in full.

Other notes include:

• RowAffected could potentially be renamed to Result - The current name limits the potential for scaling it up, and as there are likely a small amount of people using the Java client as of now, the breaking change would not be devastating. The .NET client already uses this name.
• System.Logger is used for logging.

[Mechite]

Whoops! I've forgotten to push the java.time usage upstream.

[Mechite]

So seeing as Gorse exposes many other routes in the API I'm going to implement the rest of the API in this PR as well (may as well, will be useful).

[zhenghaoz]

Thank you for your contribution. Could you rebase the main branch to fix the CI? 😄

[Mechite]

As you can see, the CI fails now due to building. In-depth notes on why are available above, but I can supply a TL;DR:

• Nothing is tested and was all blindly written (more info above)
• Tests won't work as there is a new mock server system introduced with this PR
  • Test server system utilizes TestContainers
  • TestContainers has no JPMS support - JPMS support is introduced with this PR

More notes:

• I plan to most likely try to implement the rest of the Gorse API and attach that commit here before the PR is merged
• WRT GorseFactory#116, there is a constant string zhenghaoz/gorse-in-one:0.4.14 referencing a specific version of gorse-in-one. Based on your recent commit fix integrated test #3, you probably want to have this changed to exclude the version. I'll make that change now
• TestContainers is not the ideal solution, but tends to be the cleanest to quickly implement a solution to run something for a test
  • Invoking Gorse with something like gobind seems like a cool option, but the work is not worth it just to get working unit tests, and it would be nice to implement all these testing features in the other clients
  • I might make some PRs for the other clients such as the .NET one to get all of these features ported over afterwards, as well as the asynchronous model for the client (which is a really useful thing for chaining calls if you are working on a reactive project, ie ReactiveX).

[zhenghaoz]

You could keep using docker-compose to set up the test environment if TestContainers is not ideal.

[Mechite]

You could keep using docker-compose to set up the test environment if TestContainers is not ideal.

I have a commit in progress that solves the issues by simply having a non-JPMS compatible separate module that deals with testing, most people don't directly use JPMS in their unit tests anyway (so TestContainers can work properly)

[Mechite]

That should wrap it up! I'll make another commit tomorrow to implement missing API routes that other Gorse clients seem to implement. However, if you see this PR fits your expectations you can merge it and I'll make a new PR for the upcoming changes.

I don't know if you'd like to bump the version though; the version of Gorse and all the clients seems to be matched so perhaps this just goes out with the next release of Gorse? It's ideal if its out within a month for my specific needs (or I'll just start driving with my fork otherwise) - or is there a specific place I can read the release schedule?

Thank you for being so fast to review my changes !!

EDIT - sidenote, forgot to mention - as JPMS is not supported by TestContainers, that is why I took this approach to seperate into a test module - however, due to the dependency the test module has on the client module, the tests for the client module can't depend on the test module to mock the server as it would cause a circular (which could easily be fixed by bumping the version etc) - so instead what I've done is move the tests to the test module.

This could be controversial as the test module is semantically representing "features to help people test their code that uses Gorse". This means that the tests being in there is weird since they look like tests written to test the test module and not tests written to test the client.

However, the client is so small it really doesn't matter in this case and once JPMS is adopted this can be refactored so easily as it is only one test class. I don't see the client expanding anytime soon, Gorse has such a simple paradigm for development and really expansions would probably go into the actual models used, etc, I don't expect many API expansions unless people really want a lot of customizability - testing those would probably be trivial though.

EDIT 2 - Also, I wonder, the JSON responses are using the Go convention:

{
  "Comment": "insect",
  "Labels": ["crocodilia", "cetacean", "horse"],
  "Subscribe": ["fish","dog","lion"],
  "UserId": "cat"
}

I'm not very well-aware of the Go ecosystem and if this is the convention for Go developers but, at least, while I dislike Google's approach usually, their style guide tends to prefer camelCase - at least for me this feels more standard, though snake_case is also something I've seen often.

Is this just a common thing for Go developers / deserialization libraries, was this done intentionally.. ? This is a weird one since Google maintains Go yet their convention differs?? Once again, I don't think anybody cares about a different naming convention (it really doesn't matter) but I'm not sure why this would be a standard thing in the Go community.

[Mechite]

Huh! The CI fails for an unknown reason. I'll take a look at this too (NoClassDefFound, this boggles me because I have a similar TestContainers setup working elsewhere).

[zhenghaoz]

Huh! The CI fails for an unknown reason. I'll take a look at this too (NoClassDefFound, this boggles me because I have a similar TestContainers setup working elsewhere).

Does mvn test works in your dev environment?
FYI: https://github.com/gorse-io/gorse4j/blob/main/CONTRIBUTING.md

[zhenghaoz]

That should wrap it up! I'll make another commit tomorrow to implement missing API routes that other Gorse clients seem to implement. However, if you see this PR fits your expectations you can merge it and I'll make a new PR for the upcoming changes.

I don't know if you'd like to bump the version though; the version of Gorse and all the clients seems to be matched so perhaps this just goes out with the next release of Gorse? It's ideal if its out within a month for my specific needs (or I'll just start driving with my fork otherwise) - or is there a specific place I can read the release schedule?

Thank you for being so fast to review my changes !!

EDIT - sidenote, forgot to mention - as JPMS is not supported by TestContainers, that is why I took this approach to seperate into a test module - however, due to the dependency the test module has on the client module, the tests for the client module can't depend on the test module to mock the server as it would cause a circular (which could easily be fixed by bumping the version etc) - so instead what I've done is move the tests to the test module.

This could be controversial as the test module is semantically representing "features to help people test their code that uses Gorse". This means that the tests being in there is weird since they look like tests written to test the test module and not tests written to test the client.

However, the client is so small it really doesn't matter in this case and once JPMS is adopted this can be refactored so easily as it is only one test class. I don't see the client expanding anytime soon, Gorse has such a simple paradigm for development and really expansions would probably go into the actual models used, etc, I don't expect many API expansions unless people really want a lot of customizability - testing those would probably be trivial though.

EDIT 2 - Also, I wonder, the JSON responses are using the Go convention:

{
  "Comment": "insect",
  "Labels": ["crocodilia", "cetacean", "horse"],
  "Subscribe": ["fish","dog","lion"],
  "UserId": "cat"
}

I'm not very well-aware of the Go ecosystem and if this is the convention for Go developers but, at least, while I dislike Google's approach usually, their style guide tends to prefer camelCase - at least for me this feels more standard, though snake_case is also something I've seen often.

Is this just a common thing for Go developers / deserialization libraries, was this done intentionally.. ? This is a weird one since Google maintains Go yet their convention differs?? Once again, I don't think anybody cares about a different naming convention (it really doesn't matter) but I'm not sure why this would be a standard thing in the Go community.

In the very early days, Gorse is just a toy project. So.... the json field names are decided causally. The later versions have keep compatibility.

[Mechite]

Unsure if this fixes the tests as I can't currently test it out but hopefully CI can tell us that

[Mechite]

Caused by: java.lang.IllegalArgumentException: Resource with path io/gorse/gorse4j/resources/config.toml could not be found on any of these classloaders: [jdk.internal.loader.ClassLoaders$AppClassLoader@5c29bfd]

!? - I symlinked that file. Having a look at it in the repository, we see this:

Yikes. It seems like making a symlink on Windows really isn't a good idea. I've been doing this for ages now, and a whopping two projects I've written have these symlinks. Glad nobody except me has ever attempted to build them, and this is what CI is great for.

I also noticed that .idea/codeStyles was pushed to the repository. Weird one, that, it should be excluded in the .gitignore, but it seems like Toptal really did a very specific gitignore that favours IDE specific projects - most people use things like .editorconfig etc for code style, but it seems like Toptal's gitignore is built not to break your project.

I'll add that to the gitignore, right at the top commented (breaks the elegance, but it helps people understand that it isn't included with Toptal's original file).

[Mechite]

Another note - Avaje is soon adding a support for varargs in client API definitions, e.g.,

RowAffected insertFeedback(List<Feedback> feedback);

// alternative
RowAffected insertFeedback(Feedback... feedback);

which makes it a lot easier to call the method without having to run List.of. Could make things a little more concise and fluent.
They can be overloaded, so we can add this feature at some point.

When I start working on adding the other API routes I'll add support for Stream and Set as well, Set because it's nice to have but I don't expect large sets of data thrown at gorse all at once, and Stream because it can simplify the development model, if e.g. you want to map a stream of something to a stream of gorse Users, they can pass the stream directly in instead of e.g. converting back to a list.

[zhenghaoz]

Thank you for contribution. Maybe the first important thing is to fix this CI. 🤣

You could test by mvn test in local environment.

[Mechite]

CI failed this time because the HTTP client failed to connect. Likely this is caused by either TestContainers not blocking the thread when starting a container, and Gorse therefore not being available when a client is requested in a test.

I'll address this soon! However, this seems to be the only major problem left for this PR 🥳

EDIT - I've been very busy so this is stale, but I will get to it!

[Mechite]

It has been over a month and I still have not been able to open up the time to work on this PR. However I do want to make a few notes here:

• After a quick review of my PR it is clear that whitespace is a running issue - .editorconfig files need to be made in, not just this repository, but every Gorse client and the Gorse software itself. Without them, contributors will have a hard time keeping whitespace consistent (I personally use tabs for indentation, and it seems like that might not be the method which was used throughout the repository before).
• GorseFactory members can be made private from what it looks like as they aren't referenced elsewhere (and shouldn't be)
• It is quite a good idea to work with this TestContainers system as, especially being split into the separate module, it provides an easy interface for developers to run their own mock Gorse server for testing their own code with.

[Mechite]

I am closing this PR as I have not been able to make the time to work on it, and I think it is more suitable that these changes (and more) are brought in several, smaller PRs.