Jwts.builder - audience cannot be add without calling add()

[michal-trnka]

Describe the bug
When using builder to create a token adding audience needs to be followed by and(). This behaviour is not documented and it is inconsistent (eg. claims do not need that).

To Reproduce
Try to add audience to a builder by calling builder.audience().add("my-web");
Test case:

  @Test
  public void testJwtBuilder(){
      //working
      JwtBuilder builder = Jwts.builder();
      builder.audience().add("my-web").and();
      String json = builder.compact();

      String decoded = new String(Base64.getDecoder().decode(json.split("\\.")[1]));
      assertThat(decoded).contains("aud");

      // Failing test scenario
      JwtBuilder builder2 = Jwts.builder();
      // missing and()
      builder2.audience().add("my-web");
      String json2 = builder2.compact();

      String decoded2 = new String(Base64.getDecoder().decode(json2.split("\\.")[1]));
      assertThat(decoded2).contains("aud");
  }

Expected behavior
Adding audience to a builder does not need to be followed by and()

[lhazlewood]

Hi @michal-trnka! Thanks for the issue (and especially the test case!). This is very helpful.

For what it's worth, the concept of nested builders and using and() is documented throughout the project:

• using a Jwts.builder() with the nested header() builder: https://github.com/jwtk/jjwt?tab=readme-ov-file#jwtbuilder-header
• b64 non-detatched payload example: https://github.com/jwtk/jjwt?tab=readme-ov-file#non-detached-payload-example
• Custom compression algorithm (zip() nested builder): https://github.com/jwtk/jjwt?tab=readme-ov-file#custom-compression-algorithm

Also individual JwtBuilder claims methods work like this as well, they're just delegates to the nested claims() builder. For example:

@Override
public JwtBuilder issuer(String iss) {
    return claims().issuer(iss).and();
}

But I definitely understand how it may not be clear that one must call the and() method, resulting in this unexpected behavior.

So we'll use this issue to track the work to address this. There are two solutions:

1. Change nested builder implementations to apply their changes immediately when add (and similar) methods are called.

   The challenge with this is that this is not typical builder behavior: most people expect builders to 'build' (do final completion work) when a build() method is called; (but for nested builders, the and() method is essentially the build method, it just also returns to the parent builder).

   So changing builder pattern semantics (instant change per method vs change-all-when-build-is-called) might cause confusion. It's probably worth thinking through the repercussions of changing this.

2. Keep the existing "only apply changes when and() is called" and then just be super clear about this in documentation and JavaDoc so that it's consistent and people understand the repercussions.

I think my preference is the first solution if we can make this work easily and there are no negative side-effects. I'll have to do some testing.

Thanks again for the issue!

[michal-trnka]

Many thanks for the thorough explanation!

I have just again tested claims to be sure. It accepts the following code snipped. I guess this is what made me confused most.

builder.claims().add("my-claim", "my-value");

I'd prefer option # 2. However, adding it to JavaDoc would make the behavior more transparent and would work for me too.

[lhazlewood]

@michal-trnka after looking at the code, I went with the first option in PR #917 and it worked out well - no surprises or 'penalties' to work around.

There was partly a strong reason to apply changes immediately, not the least of which is that it didn't 'hurt' anything, but also it avoids problems with method chains like:

.header().add("foo", "bar");
// sometime later:
.header().empty().critical()...

If we didn't apply changes immediately when .empty() is called, we'd have to create internal 'stack' of sorts to track changes across method invocations before the .and() method is finally called. It was much easier to avoid that and be consistent across all mutation/change methods.

[lhazlewood]

This is now released in 0.12.5. @michal-trnka Thank you for reporting the issue!

[michal-trnka]

Awesome, thanks a lot for fixing that!