java-sdk calling the server without content when submitting empty deletes and writes

[srose]

Open for this PR:

• discuss the Java changes
• add a test for the Java changes

Description

For the Java SDK make passing empty deletes and empty writes to the write command perform a server call without content

References

Part of #299 and #306
SDK PRs to come shortly

Review Checklist

• I have clicked on "allow edits by maintainers".
• I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
• The correct base branch is being used, if not main
• I have added tests to validate that the change in functionality is working as expected

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: srose / name: Sebastian Rose (95e9c96, 2348d08, 9e93eb5)

[rhamzeh]

Thanks for the PR @srose!

Apologies if the issue was worded vaguely. The intention is not to short-circuit when the field is not provided, instead it's to ensure the field is not sent to the API if it were an empty array.

This may currently already be the case, but we'd like to add tests specifically for this, and if it's not the case, fix it so that they're no longer being sent.

OpenFgaClient User Request	Sent to the API
Write(writes=[tupleA], deletes=[tupleB])	Write(writes=[tupleA], deletes=[tupleB])
Write(writes=[], deletes=[tupleA])	Write(deletes=[tupleA])
Write(writes=[tupleA, deletes=[])	Write(writes=[tupleA])
Write(writes=[], deletes=[])	Write()

[srose]

@rhamzeh, @jimmyjames thank you for the clarification, gave it a try with a test and at least the test failed without modification. Please have a look and decide if my changes are useful.

[jimmyjames]

ok, I spent a little bit of time looking at this, and tl;dr I think this SDK is already not actually sending the writes or deletes fields if not set 😄.

If we look at writeNonTransaction, it only adds the delete or write tuples to the request if they are non-null and not empty.

Since the default value for those fields will be null in that case, and the default configuration for the ObjectMapper is JsonInclude.Include.NON_NULL, those fields won't be serialized on the request.

For example, this test includes the logic from writeNonTransaction and you can see if you configure the ObjectMapper to not include null values, you'll see it is not serialized.

@Test
    public void jsonTest() throws Exception {
        List<ClientTupleKeyWithoutCondition> tupleDeletes = List.of(new ClientTupleKeyWithoutCondition()
                ._object(DEFAULT_OBJECT)
                .relation(DEFAULT_RELATION)
                .user(DEFAULT_USER));

        var request = new ClientWriteRequest().writes(Collections.emptyList()).deletes(tupleDeletes);

        ObjectMapper mapper = new ObjectMapper();
        mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);

        WriteRequest body = new WriteRequest();

        var writeTuples = request.getWrites();
        if (writeTuples != null && !writeTuples.isEmpty()) {
            body.writes(ClientTupleKey.asWriteRequestWrites(writeTuples));
        }

        var deleteTuples = request.getDeletes();
        if (deleteTuples != null && !deleteTuples.isEmpty()) {
            body.deletes(ClientTupleKeyWithoutCondition.asWriteRequestDeletes(deleteTuples));
        }

        System.out.println(mapper.writeValueAsString(body));
    }

Produces (notice no writes field):

{"deletes":{"tuple_keys":[{"user":"user:81684243-9356-4421-8fbf-a4f8d36aa31b","relation":"reader","object":"document:budget"}]}}

All that said, the added test in your PR uncovered a different potential issue 😆. After building the transactions, we call writeNonTransaction with transactions.get(0), which in your test case results in an IndexOutOfBoundsException.

Let me think on it a bit but I think I'll create a new issue for the IndexOutOfBoundsException case and perhaps the change in this PR might be a good way to address that.

Thanks again for your contribution and patience as we dig into it more!

[jimmyjames]

This may currently already be the case, but we'd like to add tests specifically for this, and if it's not the case, fix it so that they're no longer being sent.

Also, for completeness to @rhamzeh's point, I believe there are tests that cover the original reported issue cases; writeTuplesTest(), deleteTuplesTest, and write_nothingSentWhenWritesAndDeletesAreEmpty. Those tests show the empty fields marshalled as null, which would be expected given the test uses a mock so doesn't use the actual ObjectMapper configuration in ApiClient.

[jimmyjames]

Fixes #313

Thanks @srose!

[srose]

@jimmyjames thank you for your support, i think you are right on everything you wrote. I think I will try to rework the code with the IndexOutOfBounds section. Its fixed now, but I can think of an improvement.