Immutables: Define parameter ordering of inherited immutable abstract classes

[philleonard]

I have the following inheritance situation;

@Value.Immutables
public abstract class AbstractBase {
    public abstract String getId();
}

@Value.Immutables
public abstract class AbstractFoo extends AbstractBase {
  public abstract Object getX();
}

@Value.Immutables
public abstract class AbstractBar extends AbstractBase {
  public abstract Object getY();
}

which generates the of constructor methods AbstractFoo.of(x, id) and AbstractBar.of(y, id). What I would actually like is for the ability to have the base class attributes to be first in the of constructors (i.e. AbstractFoo.of(id, x) and AbstractBar.of(id, y)) without having to do the following unmaintainable parameter ordering between classes;

@Value.Immutables
public abstract class AbstractBase {
   @Value.Parameter(order = 1)
    public abstract String getId();
}

@Value.Immutables
public abstract class AbstractFoo extends AbstractBase {
  @Value.Parameter(order = 2)
  public abstract Object getX();
}

@Value.Immutables
public abstract class AbstractBar extends AbstractBase {
  @Value.Parameter(order = 2)
  public abstract Object getY();
}

(Which in fact gives the following error prone warning Annotation @Value.Parameter is superfluous when Style(allParameters = true) as I rely on allParameters = true elsewhere). I wonder if there is a way around this, or could we perhaps introduce a style parameter or class level annotation which handles ordering in the desired manner?

[Stephan202]

If this is indeed possible: maybe list the parameters of the super classes(s) first? Imagine having several subclasses all sharing some properties defined by a shared superclass (or interface...). Say, an ID and a name. It'd be nice to start each object construction by listing those first.

[elucash]

I agree that warning is misleading in this case. I think we can change order of attributes for supertypes first, but I worry about if users are get used to old functionality (I do consider style switch, but still). Moreover for complex supertypes hierarchies there are no stable supertype linearization implemented in Immutables and further complicated by compilers might return supertypes in different order across consecutive compilation runs. As I already outlined elsewhere this is very brittle functionality: generating constructor from parameters collected across supertypes, where builder would be better option.

The best solution is to redeclare parameters in subtypes, because it will produce stable and predictable constructor parameter lists.

@Value.Immutable
public abstract class AbstractBase {
    public abstract String getId();
}

@Value.Immutable
public abstract class AbstractFoo extends AbstractBase {
  @Override public abstract String getId();
  public abstract Object getX();
}

@Value.Immutable
public abstract class AbstractBar extends AbstractBase {
  @Override public abstract String getId();
  public abstract Object getY();
}

I do understand you want less repetition, but it's seems that there should be a reasonable tradeoff.

[scranen]

I'm in a similar situation, but my base class has ~15 fields and I have >5 derived classes. That's quite a lot of repetition. Maybe I can propose a solution that will not break backwards compatibility and still allow users to use inheritance.

Let's take a complicated example:

interface X {
   @Parameter int x();
}
interface Y {
    @Parameter int y();
}
interface A extends X {
    @Parameter int z();
}
interface B extends Y {
    @Parameter int z();
}
@Immutable
interface Point extends  A, B {}

Could we not introduce an annotation @InheritProperties(classes = { X.class, ... }) that can specify the order in which to process parent properties? The semantics would be:

• Without this annotation, nothing changes
• With the annotation, the code generator requires that the list of classes contains at least all direct super types. The classes parameter can be optional in the case of single inheritance.
• With the annotation, super types are processed in the specified order, and properties are processed in declaration order. If a super type again has multiple super types, the @InheritProperties annotation is required to be present on that class too.

For the toy example (we assume allParameters = true):

@Immutable
@InheritProperties({A.class, B.class})
interface Point extends A, B {}

yields a class with members (and constructor parameters) x(), z(), y(), in that order. If you want x(), y(), z(), then you specify e.g.:

@Immutable
@InheritProperties({X.class, Y.class, A.class, B.class})
interface Point extends A, B {}

I believe this approach resolves the drawbacks outlined in #292. In particular, for the "extending from 3rd party immutables classes" argument: by explicitly putting the annotation on the class, you accept the risk of the underlying class changing property order if it's a 3rd party class.

[matt-isaman]

I am also having issues with this.

I have a base response type for an API that gets overloaded for all the various responses:

public abstract class Response<T extends ResponseContent> implements Serializable {

    private static final long serialVersionUID = 1L;

    public abstract Optional<T> getContent();

    public abstract Optional<AnError> getError();
}

This class is extended in many places like so:

@Immutable
@Style(typeAbstract = "*Defn",
       get = { "is*", "get*" },
       init = "with*",
       typeImmutable = "*",
       depluralize = true,
       visibility = PUBLIC,
       deepImmutablesDetection = true,
       privateNoargConstructor = true,
       throwForInvalidImmutableState = InvalidObjectException.class,
       strictBuilder = true,
       jdkOnly = true,
       redactedMask = "********",
       allParameters = true,
       defaults = @Immutable(builder = false, copy = false))
abstract class GetAThingResponseDefn extends Response<AThing> {

    private static final long serialVersionUID = 1L;
}

The issue I am having is that on repeated builds the property orders get flipped around. So all of my calls to construct instances of these objects fail the build transiently. So I attempted to fix this by updating the response class to the following:

public abstract class Response<T extends ResponseContent> implements Serializable {

    private static final long serialVersionUID = 1L;

    @Parameter(order = 1)
    public abstract Optional<T> getContent();

    @Parameter(order = 2)
    public abstract Optional<AnError> getError();
}

After doing this the transient build breaks stop happening but I get all these annoying warnings. I could do as suggested and override the methods in every sub-class of Response but I would rather there be some other way to resolve this.

[matt-isaman]

Ran into this again in a slightly more annoying situation.

I have this in the base class

    @Default
    public UUID getUUID() {
        return UUID.randomUUID();
    }

Which then requires that in all implementing classes I include the following:

    @Default
    @Override
    public UUID getUUID() {
        return super.getRequestId();
    }

Not doing the above results in the warning:

warning: (immutables:subtype) Constructor parameters should be better defined on the same level of inheritance hierarchy, otherwise generated constructor API would be unstable: parameter list can change the order of arguments. It is better redeclare (override) each inherited attribute parameter in this abstract value type to avoid this warning. Or better have constructor parameters defined by only single supertype.

Add this to all of the other inherited abstract methods I need to override and the sub-classes are looking kind of funny. Can we revisit this issue for an upcoming release? I would love it if there was a stable ordering for the parameters so that I could remove all of these overrides.

[elucash]

thank you for reporting the new issue with constructor parameters! Can you, please, provide a more complete example, so I can better understand the issue.

[matt-isaman]

Sorry for taking so long to respond. Got super busy for a bit there. Here is an example of what I am referring to (using immutables 2.7.3 and java 11 if those specifics matter):

package scratch;

import java.util.UUID;
import org.immutables.value.Value.Default;

public abstract class Request {

    @Default
    public UUID getRequestUUID() {
        return UUID.randomUUID();
    }
}

And:

package scratch;

import org.immutables.value.Value.Default;

public abstract class PaginatedRequest extends Request {

    @Default
    public int getPageNumber() {
        return 0;
    }

    @Default
    public int getPageSize() {
        return 100;
    }
}

And finally:

package scratch;

import java.util.Optional;
import org.immutables.value.Value.Immutable;
import org.immutables.value.Value.Style;

@Immutable
@Style(allParameters = true, defaults = @Immutable(builder = false, copy = false))
abstract class ExampleRequest extends PaginatedRequest {

    public abstract String getS();
    public abstract Optional<Integer> getOptI();
}

Gets me the warning:
(immutables:subtype) Constructor parameters should be better defined on the same level of inheritance hierarchy, otherwise generated constructor API would be unstable: parameter list can change the order of arguments. It is better redeclare (override) each inherited attribute parameter in this abstract value type to avoid this warning. Or better have constructor parameters defined by only single supertype.

I have a request type hierarchy for my service where all requests have a uuid, paginated requests have a pagination object (just fields in this example to keep it smaller), and the specific request objects have how many ever fields they need.

To avoid the warning, and the dangers of the parameters showing up in a different order, I have to add the following to every immutable class that inherits from PaginatedRequest:

    @Override
    @Default
    public int getPageNumber() {
        return super.getPageNumber();
    }

    @Override
    @Default
    public int getPageSize() {
        return super.getPageSize();
    }

    @Override
    @Default
    public UUID getRequestUUID() {
        return super.getRequestUUID();
    }

As more gets added to the based classes more overridden methods get added to the immutable child classes. Not using the factory method builder avoids this, but for a reasonably small set of fields I like using the factory methods and don't like having to add the above overridden methods.