Skip to content
This repository has been archived by the owner on Oct 16, 2024. It is now read-only.

Support derived fields #441

Open
asjp1970 opened this issue Mar 4, 2021 · 4 comments
Open

Support derived fields #441

asjp1970 opened this issue Mar 4, 2021 · 4 comments

Comments

@asjp1970
Copy link

asjp1970 commented Mar 4, 2021

I thought of using the Builder pattern to create a rather complex Object containing a chain of certificates and keys. Our intention is to take from clients of the class the burden to provide specific cryptographic parameters; those parameters (algorithms, sizes of keys, salts, etc.) are always fixed and we want everything set under the hood.

To initialize my object I just need a list of parameters, more or less long, but simple: expiration dates, passwords to do PBEncryption, etc. JCA specific attributes (like private keys and X509 certs) can perfectly remain Optional, since I will provide them for clients using the handful of the other (required parameters).

The problem I've found with FreeBuilder is that I cannot hook in the build() method to build the other (complicated) stuff on the client's behalf. Surely is not a problem, and if it is a conscious decision, please disregard the rest of this idea.

Something like this:

In the generated Builder:

...
public MyClass_Builder build() {
    Preconditions.checkState(_unsetProperties.isEmpty(), "Not set: %s", _unsetProperties);
    buildHook()
    return new Value(this);
  }
...
/**
   * Hook to introduce additional build actions before {@link MyClass_Builder#build()} returns
   * the value of this builder.
   *
   */
protected void buildHook(){}

And in the inner Builder:

@Override
protected void Builder buildHook() {
    // Do here additional stuff to build Optional parameters
  }

The reasons why I think this could be useful are:

  • NOT to allow clients of MyClass access the Optional properties BEFORE I have made sure they have been properly set. I only keep them optional to hide from clients the complexity of providing those properties.
  • It still relies on FreeBuilder checks of the other mandatory properties.
  • There is no way in MyClass_Builder to construct the other parts of it by overriding the setMethods... other than reproducing in that class something similar to the control of the EnumSet, by overriding all setters for mandatory methods, and then only when all of them have been invoked, take the additional (complex) build steps... That would not worth using FreeBuilder.
  • Still the other nice features of FreeBuilder (that we are using) would be available: mapping and merging.
@alicederyn
Copy link
Collaborator

It sounds like these aren't really Optional parameters at all, since they would never be missing on the built object, so a proper solution wouldn't mark them as Optional (and hence the checkState would fail in your example build method).

I feel like a closer match to your needs would be either:

  • the ability to provide a default value at build time rather than init time, or
  • the ability to provide a derived value at build time

(Which would be better depends on whether you want users to have the ability to override these properties or not.)

You could approximate the second by implementing the derived getters yourself, fetching from fields that you construct in a post-build method:

@FreeBuilder
abstract class MyType {
  private String foo;
  public abstract int bar();
  private void postBuild() {
    foo = "bar=" + bar;
  }
  public String foo() {
    return foo;
  }
  public class Builder extends MyType_Builder {
    public MyType build() {
      MyType value = super.build();
      value.postBuild();
      return value;
    }
  }

The only problem here is the fields aren't final, which leads to unfortunate concurrency side-effects. Supporting this properly in FreeBuilder would be better.

I can't see a clean way to approximate the first in a way that interacts properly with toBuilder/mergeFrom — but I can't see a clean way to implement it under the hood either! Since there's no way to know after calling build whether the object took the default value or not, there's no way to avoid copying it verbatim to other objects, even if the fields it was derived from change.

Which of these two options most closely matches what you want (if either)? And if it's the default value one, how would you expect something like myObject.toBuilder().bar(10).build() would affect fields that were originally derived from bar?

@asjp1970
Copy link
Author

asjp1970 commented Mar 4, 2021

Yeap, the fields not being final is an issue.
In my case the default option is not feasible. And also has the drawback of having to partially re-build all the other properties affected by myObject.toBuilder().bar(10).build() , as you pointed out.
You are totally right that the other fields are not Optional: they must be part of the build, but we wanted to avoid clients depending from all the JCA class jargon.

Anyway, thanks for your time and I think you can close this issue; I just was after sharing the idea with the expert.

@alicederyn
Copy link
Collaborator

alicederyn commented Mar 4, 2021

I'd like to leave this open as a potential enhancement for future work. It's unlikely to get worked on soon, but if you felt able to do this yourself, we welcome contributions.

One way of achieving this with decent feature karma, that might unlock other use-cases, would be to allow the abstract base class to declare parameters on its constructor that line up with the getter fields. Then it could create whatever final fields it wanted.

@FreeBuilder
abstract class MyType {
  private final String foo;
  public abstract int bar();
  MyType(int bar) {
    foo = "bar=" + bar;
  }
  public String foo() {
    return foo;
  }
  public class Builder extends MyType_Builder { }
}

This wouldn't work nicely with partials, though.

Alternatively, we could provide a more targeted "derive field X" method:

@FreeBuilder
interface MyType {
  String foo();
  int bar();
  public class Builder extends MyType_Builder {
    String foo() {
      // The presence of this method would suppress FreeBuilder's normal property
      // generation for the foo field.
      // The generated code that calls this would have to catch IllegalStateExceptions
      // in buildPartial, and turn them into UnsupportedOperationExceptions when foo()
      // is called
      return "bar=" + bar();
    }
  }
}

Not sure if it's best to treat "I made a package-protected getter" as code for "this is a derived property", or have some other convention. I have some concerns about dependencies between these fields that might be better solved with one approach or another. For instance, the convention could be "declare a static buildFoo method that takes in all the required properties as parameters", which would give FreeBuilder an explicit dependency graph to let it figure out which methods it needs to call first — and which ones it can't satisfy, in the case of partials, rather than using try/catch.

@FreeBuilder
interface MyType {
  String foo();
  int bar();
  public class Builder extends MyType_Builder {
    static String deriveFoo(int bar) {
      // The presence of this method would suppress FreeBuilder's normal property
      // generation for the foo field.
      return "bar=" + bar;
    }
  }
}

Using a magic name like this has a small risk of accidentally conflicting with existing code, so we might consider an explicit new annotation for such methods (which would have the advantage of being more self-documenting).

Interested to hear any other suggestions.

@alicederyn alicederyn changed the title Provide a way to hook to the build() method Support derived fields Mar 4, 2021
@alicederyn
Copy link
Collaborator

alicederyn commented Jul 23, 2021

Drive-by noting another syntax option that occurred to me:

@FreeBuilder
interface MyType {
  int bar()
  @Cached(lazy=false) default String foo() {
    return "bar=" + bar();
  }
  public class Builder extends MyType_Builder {}
}

The implementation can call the default method once and cache the result (either eagerly or lazily). This avoids magic methods on the Builder, but (for the eager implementation) would need try/catch on Partials. One possible downside is that non-FreeBuilder implementations of this interface are not constrained to implement the caching, and no compile errors will occur if they do not.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants