Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show some love ❤️ to the Java SDK #476

Open
piotrooo opened this issue Jan 25, 2025 · 4 comments
Open

Show some love ❤️ to the Java SDK #476

piotrooo opened this issue Jan 25, 2025 · 4 comments

Comments

@piotrooo
Copy link
Contributor

Motivation

Recently I was exploring the Java SDK, and I noticed that it is somewhat difficult to maintain, especially compared to the Python, Go, or JS SDKs, which are well-maintained. Several libraries are outdated - for instance, the HTTP mocking library, which has a bug when handling multiple async requests. Additionally, there are tons of duplications, and Javadocs are almost non-existent.

I have identified the following areas that could be improved, and I'd like to hear your thoughts on the proposed ideas.

I am willing to take the lead on implementing these changes.

Tests

  • Adopt WireMock: This library is a standard for mocking API responses in enterprise projects and is highly reliable for such tasks.

  • Switch to AssertJ: AssertJ is another enterprise standard. Adopting it would help us avoid overly complex assertions like this:

ClientBatchCheckSingleResponse response1 = response.getResult().stream()
        .filter(r -> r.getCorrelationId().equals("cor-1"))
        .findFirst()
        .orElse(null);
assertNotNull(response1);
assertTrue(response1.isAllowed());
assertEquals("user:81684243-9356-4421-8fbf-a4f8d36aa31b", response1.getRequest().getUser());

ClientBatchCheckSingleResponse response2 = response.getResult().stream()
        .filter(r -> r.getCorrelationId().equals("cor-2"))
        .findFirst()
        .orElse(null);
assertNotNull(response2);
assertFalse(response2.isAllowed());
assertEquals("folder:product", response2.getRequest().getUser());
  • Revive Integration Tests: Effort should be put into "resurrecting" integration tests. These should be connected with Testcontainers as they are currently, but additional tests should be added.

HTTP Client

First of all, I’d like to ask: why was the Java HTTP Client chosen? Were there specific reasons or constraints that led to this decision?

I believe a better alternative would be Apache HttpComponents HttpClient. However, I acknowledge that I might not be aware of all the pros and cons considered when this library was selected.

Template structure

Currently, all template files are located in a single directory and prefixed by their destination. I suggest adapting the directory structure to better organize the templates, similar to how it’s done in other SDKs.

Java baseline

The SDK currently uses Java 11 as its baseline version. However, most major players, such as Spring and JUnit, have set their baseline version to Java 17. It might be a good idea to align with this trend since Java 11 is no longer supported.

Moreover, our spring-boot-starter also uses Java 17, which further supports the case for upgrading the baseline version.

@rhamzeh
Copy link
Member

rhamzeh commented Feb 5, 2025

Thanks @piotrooo!

Will wait for @jimmyjames's thoughts, but here are some of mine:

  • Re: Wiremock and TestContainers - yes this is something we want not just for the Java SDK, but for the rest of the SDKs too. However we would like to standardize the set of tests across the SDKs, so ideally whatever setup we have for Java can be re-used for the rest. Long term (not now), we have an idea about having the tests written in a declarative, language agnostic manner (e.g. in cucumber), and having the test runners be implemented per language - that would allow us to ensure the same feature set is being respected across SDKs and we get out from some of the issues where bugs are fixed in one SDK but not another. We would welcome any change in that direction, however if you want to take on this task or part of it, may I ask you to create an issue and expand on how you're proposing to do it before actually implementing it? Just so that you don't do a lot of work that we may object to or want to change.
  • Re: AssertJ - Can you elaborate on how assertions would be after we include it? Normally we are very hesitant about any external dependencies across the SDKs and try to keep them to a minimum. If it has a strong benefit, we can include it - but we just want to be as conservative as possible.
  • Re: Java HTTP Client - that was my decision (you can see it here: Java SDK: Choosing an HTTP client library #157), and it may not have been the best one. However, again I'll add to my reservation for adopting external dependencies. E.g. if going to Java 17+ w/ the existing client solves issues, I'd rather do that than choose another dependency.
    In an ideal world, across the SDKs, we'd adopt an interface for the HTTP Client and let folks bring their own if they so wish. How hard that is to implement for us is a different topic.
    That said, the decision can be reversed - what we care about are:
    • Security: External dependencies would mean we need to be very vigilant about dependency updates, if any new code is introduced, and make sure to bump up as soon as we see a security issue or a CVE
    • Performance: In FGA, latency is King and Queen - especially as folks will be calling endpoints like check, 10s of thousands of times per second and more
    • Maintainability: Ease of writing and understanding the code, how easy it is to mock and test, etc..
  • Re: Template structure
    Yep - makes sense, some of the other SDKs suffer from this too, but we've been slowly migrating them away
  • Re: Java version - Back when we started (2023), folks were asking for support for Java < 11 - so even making that the minimum version was a stretch, and most stats showed the absolute majority were on Java 8 or 11. The 2024 stats show Java 17 had a huge adoption, though Java 8 + 11 still seem to make >50%. We can discuss whether it's time to move past Java 11 - but we will need to check-in with our community to see who we would be leaving behind.
    Sources:

@jimmyjames
Copy link
Contributor

Thanks @piotrooo! This is a good and well thought-out list!

I don't have much to add to @rhamzeh's thoughtful response. I generally like AssertJ and WireMock but echo @rhamzeh's points about long-term test framework intentions and the desire to minimize dependencies. With regard to the baseline Java version, I also would like to get us to Java 17 for the reasons you laid out, but we do have to be considerate of where our customers are. As you mentioned, there's reason for optimism with Spring and JUnit requiring Java17 (though they also maintain other version streams with older Java requirements, and we're not there yet).

So, I think we can agree that some more immediately actionable items are:

  • updated template structure
  • added/improved javadocs

Some of the other areas for improvement will need a bit more thought and planning.

@rhamzeh
Copy link
Member

rhamzeh commented Feb 5, 2025

Possibly relevant tickets:

@piotrooo
Copy link
Contributor Author

piotrooo commented Feb 5, 2025

I'll try to merge your response, @rhamzeh, and yours, @jimmyjames, and respond to all questions or uncertainties.

yes this is something we want not just for the Java SDK, but for the rest of the SDKs too. However we would like to standardize the set of tests across the SDKs, so ideally whatever setup we have for Java can be re-used for the rest. Long term (not now), we have an idea about having the tests written in a declarative, language agnostic manner (e.g. in cucumber), and having the test runners be implemented per language - that would allow us to ensure the same feature set is being respected across SDKs and we get out from some of the issues where bugs are fixed in one SDK but not another.

Introducing something general like Cucumber could be pretty neat, I guess, and I totally agree with something like that should be applied across all SDKs.

Java could be a great proving ground to test those solutions, as it's well integrated with both WireMock and Testcontainers.

We would welcome any change in that direction, however if you want to take on this task or part of it, may I ask you to create an issue and expand on how you're proposing to do it before actually implementing it? Just so that you don't do a lot of work that we may object to or want to change.

That should definitely be a new task. I'll try to break it down at the end of this response.


Can you elaborate on how assertions would be after we include it? Normally we are very hesitant about any external dependencies across the SDKs and try to keep them to a minimum. If it has a strong benefit, we can include it - but we just want to be as conservative as possible.

I totally understand your hesitation regarding new libraries, but in this case, I think there's notting to worry about. AssertJ has been adopted by many organizations and is actively maintained.

As @jimmyjames mentioned:

I generally like AssertJ

I also like it a lot. @rhamzeh, you asked about the benefit. The trigger for proposing this change came from a specific type of assertion, which, for me as an AssertJ user, was a nightmare.

ClientBatchCheckSingleResponse response1 = response.getResult().stream()
        .filter(r -> r.getCorrelationId().equals("cor-1"))
        .findFirst()
        .orElse(null);
assertNotNull(response1);
assertTrue(response1.isAllowed());
assertEquals(
        "user:81684243-9356-4421-8fbf-a4f8d36aa31b",
        response1.getRequest().getUser());

ClientBatchCheckSingleResponse response2 = response.getResult().stream()
        .filter(r -> r.getCorrelationId().equals("cor-2"))
        .findFirst()
        .orElse(null);
assertNotNull(response2);
assertFalse(response2.isAllowed());
assertEquals("folder:product", response2.getRequest().getUser());

In AssertJ, I can write assertion like this:

assertThat(response.getResult())
        .extracting(ClientBatchCheckSingleResponse::isAllowed, r -> r.getRequest().getUser())
        .containsExactlyInAnyOrder(
                tuple(true, "folder:product"),
                tuple(false, "user:81684243-9356-4421-8fbf-a4f8d36aa31b")
        );

This is much, much cleaner.

WireMock but echo @rhamzeh's points about long-term test framework intentions and the desire to minimize dependencies

For me, 'unit tests' and AssertJ are completely different topics from WireMock and 'integration tests'.


However, again I'll add to my reservation for adopting external dependencies. E.g. if going to Java 17+ w/ the existing client solves issues, I'd rather do that than choose another dependency.

Ok, totally fine. Let's leave it for now. Maybe we shouldn't invest too much in that area , but rather focus on preparing a way to, as you mentioned, 'adopt an interface for the HTTP Client and let folks bring their own if they so which'.


Performance: In FGA, latency is King and Queen - especially as folks will be calling endpoints like check, 10s of thousands of times per second and more

At this point, newer versions of Java go into play. With each version, Java developers deliver better performance. And if it's as you beautifully said, King & Queen, this migration should be seriously considered.

Keep in mind that you will never achieve the same performance in old, unsupported Java versions as you would with the latest LTS. Customers should be aware of this — perhaps it could even be a good trigger for them to upgrade their Java version.


From this list of tasks, we should create new issues (in order):

  • Update the template structure
  • Introduce AssertJ, polish, review and organize tests
  • Adopt WireMock and Testcontainers
  • Improve Javadocs
  • Consider what kind of static analysis we should introduce

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

No branches or pull requests

3 participants