Immuclient Add support of preconditions during verifySet

[artiomi]

Add support of KeyMustExistPrecondition, KeyMustNotExistPrecondition and KeyNotModifiedAfterTXPrecondition during execution of verifiedSet

[vchaindz]

Thanks for the contribution! The overall approach looks good — the proto already supports SetRequest.preconditions, the old verifiedSet(byte[], byte[]) cleanly delegates to the new overload, and it compiles fine against master. A few things I'd like to see addressed before merge, plus some smaller suggestions.

Should fix

1. withPreconditions(null) blows up at build() — the VerifySetOptions constructor calls Collections.unmodifiableList(builder.preconditions), which throws an NPE if someone passes null. That also makes the preconditions != null check in havePreconditions() dead code. Easiest fix: have the builder fall back to an empty list (or throw a clear IAE) when null is passed.

2. No key/value validation in the builder — build() happily accepts a null key or value, and since Utils.toByteString(null) silently returns ByteString.EMPTY, a misconfigured options object would end up writing an empty key instead of failing fast. A null check in build() would catch this early.

3. Javadoc on the new verifiedSet(VerifySetOptions) got mangled — "Commits a change of a value for a single key. server-provided proof validation." reads like a sentence lost half of itself in a copy-paste. Should probably be "Equivalent to set but with additional server-provided proof validation" like the other overloads.

4. Missing license headers — Precondition.java and VerifySetOptions.java are missing the Apache 2.0 header that every other source file in the repo carries.

5. Copy-paste bug in test t6 — the result of the second verifiedSet call is discarded, so the assertion "The result of second verifiedSet must not be null" is re-checking the first txHdr.

6. Exact error message assertions are brittle — the tests compare full strings like "FAILED_PRECONDITION: precondition failed: KeyMustNotExist", which will break the moment the server tweaks its wording. Asserting exception.getStatus().getCode() == Status.Code.FAILED_PRECONDITION (and maybe a contains() on the precondition name) would be more resilient.

Suggestions / nits

• havePreconditions() → hasPreconditions() reads more naturally; and VerifiedSetOptions might be a better name to match the verifiedSet method it belongs to.
• Precondition.toProto() uses ByteString.copyFrom directly, while the rest of the codebase goes through Utils.toByteString — worth keeping consistent. Same file also mixes key and this.key.
• The if (options.havePreconditions()) guard plus the stream in ImmuClient isn't needed — addAllPreconditions on an empty list is a no-op, so you can just always add them.
• The builder could use a withKey(String) / withValue(String) convenience, given Precondition already offers of(String) overloads.
• Indentation in test t4 is off around the VerifySetOptions options = ... block.
• A note rather than a request: precondition failures surface as a raw io.grpc.StatusRuntimeException, which leaks the transport layer into the API. That's partly consistent with existing SDK behavior, but might be worth wrapping in a typed exception at some point.
• Only verifiedSet gains preconditions here, while plain set/setAll (which the proto also supports) don't. Fine for the scope of this PR, just flagging the asymmetry.

Nothing blocking correctness-wise on the happy path — mostly hardening and polish. Happy to re-review once updated.

[artiomi]

Hi @vchaindz thank you for your detailed review, I have submitted suggested changes.
One note on

2. No key/value validation in the builder — build() happily accepts a null key or value, and since Utils.toByteString(null) silently returns ByteString.EMPTY, a misconfigured options object would end up writing an empty key instead of failing fast. A null check in build() would catch this early.

I have added null check for key, but didn't add it for value. I have noted that currently is possible to pass value as null . I'm afraid that adding this check can break existing clients.

[vchaindz]

Hi @artiomi, thanks for the quick turnaround — I went through the updated changes and this looks much better. All the points from my first review are addressed: the null-safe withPreconditions, the license headers, the fixed javadoc, the corrected t6 test, and the switch to a typed FailedPreconditionException with contains()-based assertions instead of full-string matching. I also like that you went a step further and made SetOptions work for plain set() too — that nicely resolves the asymmetry I had flagged.

Regarding your note on the value null check: good thinking, and you're right. Passing a null value currently works (it's written as an empty value), so adding a check there would indeed break existing clients. Leaving value nullable is the correct call.

Three small things I'd still like to see before merge:

1. Null-key behavior change on the existing set methods — since set(byte[], byte[]) / set(String, byte[]) now go through the SetOptions builder, a null key throws an NPE where it previously (silently) wrote an empty key. I actually think fail-fast is the better behavior here, but it does change existing methods, so let's make sure it's intentional and gets a mention in the release notes.

2. Wildcard import — ImmuClient.java now has import io.grpc.*; where the rest of the codebase uses explicit imports. Please spell them out again (including the new io.grpc.Status).

3. SetOptions.hasPreconditions() is now dead code — after removing the guard in ImmuClient, nothing calls it anymore, and the preconditions != null part can never be false given the constructor. Either drop the method or keep it as public API and remove the redundant null check.

Nothing blocking correctness-wise — once these three are sorted this is good to merge. Thanks again for the contribution!

[artiomi]

Hi @vchaindz thank you for you comments. I have fixed the issues mentioned in steps #2 & #3.
I have concern about issue #1, what do you mean by :" so let's make sure it's intentional and gets a mention in the release notes." ?

Thanks,
Artiom

[vchaindz]

Hi @artiomi, sure, let me clarify — there's actually no code change being asked for in #1.

What I meant: since set(byte[], byte[]) and set(String, byte[]) now go through the SetOptions builder, their behavior for a null key changed. Before your PR, immuClient.set(null, value) went through silently — Utils.toByteString(null) returns ByteString.EMPTY, so it actually wrote an entry with an empty key. After your PR, the same call throws an NPE ("'key' can't be null") before anything hits the server.

I think the new behavior is the right one — failing fast beats silently writing a broken entry. But it's technically a breaking change for the existing methods: any client out there that (probably by accident) passes a null key will start getting exceptions after upgrading.

So the two things I meant were:

• "intentional" — just wanted to confirm you're aware the null-check now applies to the old set methods too, not only the new set(SetOptions) overload. Sounds like it was deliberate, which is fine.
• "release notes" — that's on our side: we'll flag the change in the changelog of the next release so nobody gets surprised. Nothing for you to do there.

Also had a look at your latest push — #2 and #3 both look good, imports are explicit again and hasPreconditions() is gone. So from my side this is good to merge. Thanks again!

[artiomi]

Hi @vchaindz
Thank you for your detailed answer, just want to confirm that I'm aware that current implementation of set(SetOptions) will impact existing clients by throwing NPE if null key will be passed.

Artiom