[Lokowandtg]

Implemented with the help of Claude AI.

[ZPascal]

Thanks for this, the structure follows the other v3 resources nicely and the unit tests pass for me. I left a few comments inline. The first three are the ones I would fix before merging, the rest are smaller things.

[ZPascal]

I think there is no way to clear the custom error message with this. The ObjectMapper skips null values (NON_NULL), so customErrorMessage(null) just leaves the field out of the PATCH body, and the server keeps the old message. From the client side "don't touch it" and "clear it" look the same.

Maybe we could do one of these:

• Treat an empty string as "clear" and send null for it, e.g. a small @JsonProperty getter that maps "" to null with @JsonInclude(ALWAYS) on that field.
• Or use an Optional<String>-like wrapper so we can tell unset from explicit null.

The first one is probably simpler. Either way it would be good to add a test for the clear case.

[ZPascal]

This class and _GetFeatureFlagResponse have the same five fields, and the list response uses GetFeatureFlagResponse for its items. So a new field has to be added in two places, and the list items are typed as "Get response" which reads a bit odd.

Other v3 resources (e.g. stacks) have an abstract base class (Stack) that the get/update responses and the *Resource extend. Could we do the same here with a FeatureFlag base and a _FeatureFlagResource, and let _ListFeatureFlagsResponse use FeatureFlagResource? It would also let people write code that takes "a feature flag" without caring where it came from.

[ZPascal]

A couple of things in this test:

1. The result of the restore call is thrown away by thenReturn(...), so the test never checks that the flag really went back to its original value. The v2 test asserts that. Since this is a global flag, a failed restore would leave the environment changed for other tests.
2. toggled.getEnabled() != original.getEnabled() compares two Boolean objects by reference. It works today because of the cached TRUE/FALSE, but !Objects.equals(...) is safer.

Suggestion: use .flatMap(restored -> ...) and assert on restored.getEnabled() equals original.getEnabled(). Wrapping the toggle in a doFinally (or onErrorResume that restores and rethrows) would also make sure the flag is reset if something fails in between.

[ZPascal]

The list endpoint also has an updated_ats filter in the v3 docs (please double check, I am going from memory). Right now only the generic paging options are available here. If it is there, it would be nice to add it with the usual @FilterParameter("updated_ats") getter, like other list requests do. If you want to keep this PR small, a follow-up is fine too.

[ZPascal]

This null check looks right to me, but it has nothing to do with feature flags. Could it go into its own PR, or at least get a small test (an upload with only resources and no bits) so it does not regress? Also the copyright year in the header is not updated for this file.

[ZPascal]

Small thing: name("") is accepted here. The request then goes to /v3/feature_flags/, which returns the list, and you get a confusing mapping error about missing name and enabled. The same is true for _UpdateFeatureFlagRequest.

A check like this would fail early:

@Value.Check
void check() {
    if (getName().isEmpty()) {
        throw new IllegalStateException("name must not be empty");
    }
}

Also, line 24 has some trailing whitespace in the Javadoc. Spotless skips the _*.java files so it won't catch it.

[ZPascal]

At the moment you can build an update request with neither enabled nor customErrorMessage, and validNoFields in the test even checks for that. This sends {} and gets a normal 200 back, so if someone forgets to set a field it silently does nothing.

Would it make sense to fail in a @Value.Check when both are null? If you prefer to keep it allowed because the API accepts it, that is ok too, but then a short note in the Javadoc would help.

[ZPascal]

The unit tests only cover the happy path. It would be good to have:

• a list test with page/perPage/orderBy so the query string is checked,
• an error test for get/update (404 for an unknown flag, 422 for a message that is too long). There are fixtures for that in other v3 reactor tests that can be copied.

The integration test also never sets customErrorMessage, so that mapping is only checked against the canned JSON. Setting a message in the update test and checking it comes back would cover it.

[ZPascal]

Nit: the other classes in this package (StacksTest, UsersTest, ...) are public final class. Maybe do the same here.

[ZPascal]

Just a heads-up, no change needed: adding featureFlagsV3() as an abstract method means anyone who implements CloudFoundryClient themselves (test doubles, wrappers) will get a compile error on upgrade. That is the same as with the other xxxV3() accessors, but it may be worth a line in the release notes.