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

srose commented 7 months ago

Open for this PR:

[x] discuss the Java changes
[x] 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 https://github.com/openfga/sdk-generator/issues/299 and https://github.com/openfga/sdk-generator/issues/306 SDK PRs to come shortly

Review Checklist

[x] 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]
[x] The correct base branch is being used, if not main
[x] I have added tests to validate that the change in functionality is working as expected

linux-foundation-easycla[bot] commented 7 months ago

The committers listed above are authorized under a signed CLA.

:white_check_mark: login: srose / name: Sebastian Rose (95e9c96a83993282c3e2f7bfda340973e2d4bda8, 2348d08db103fc406ab263b2bdc9dbca3d81dde7, 9e93eb5e66b302aa141e42de8aedc2a88b9790f0)

rhamzeh commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

Fixes #313

Thanks @srose!

srose commented 7 months ago

@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.

openfga / sdk-generator