@filiphr I've tested your proposed solution this morning, and it is getting there.

However, I have identified 2 problems that we need to resolve:

The default value
In a Java generated protobuf stub ("protobuf side") there are 2 special values, the default contant with value 0 ( and named XXX_UNSPECIFIED according to the style guide) and the UNRECOGNIZED constant with value -1 meaning that the parser did not understand the underlying int value received.

This means that there are 2 enum constants that map to MappingConstants.NULL.

On the "java side" there are no such values, but standard practice is to not populate the field at all ("null")

The current generated implementation picks one of these (seems to be the last parsed) for the generated code, and that is UNRECOGNIZED.

This however is an illegal value to set, and at runtime the stub will throw an exception;

  public final int getNumber() {
    if (this == UNRECOGNIZED) {
      throw new java.lang.IllegalArgumentException(
          "Can't get the number of an unknown enum value.");
    }
    return value;
  }

Therefore in order to make it correct the EnumNamingStrategy should expose something like
public String getDefaultEnumConstant(TypeElement enumType)
in order for the SPI implementation to handle this.

NullValueCheckStrategy
If a @ValueMapping with source=MappingConstants.NULL is defined and the mapper is using NullValueCheckStrategy.ALWAYS, the NullValueCheckStategy will check the source value for null before applying the @ValueMapping, effectively masking the defined mapping.

@filiphr could you shortly summarize the difference from these changes with the one from #2089?
Both belongs to enums and also both belong somehow to namings

Thanks for the feedback @seime. I have some questions:

The default value

So you are saying that nothing should map to UNRECOGNIZED and UNRECOGNIZED should map to null? Can you please show me an example generated code of how it is expected for protobuf?

NullValueCheckStrategy

Not following here. If you defined @ValueMapping with source=MappingConstants.NULL you are explicitly defining the mapping when source is null. It should not apply the one from the strategy. I think I added tests for that

@chris922 the difference between this and #2089 is that for the previous PR you have to explicitly implement how you want to do a transformation.

e.g.

@EnunMapping(nameTransformationStrategy = "PREFIX", configuration = "CUSTOM_")

This allows defining a prefix, suffix, etc. on a case by case basis.

With this PR you can implement a custom name based strategy based on certain rules. For example the style guide for Protobuf.

Okay, thanks for the clarification. Then I got it correctly.

Basically #2089 is

on a case by case basis.

vs. here

you can implement a custom name based strategy based on certain rules

.. that will be used for all enum mappings

So you are saying that nothing should map to UNRECOGNIZED and UNRECOGNIZED should map to null? Can you please show me an example generated code of how it is expected for protobuf?

Correct @filiphr . If an enum has not been set, it will have the value 0.
UNRECOGNIZED is used when the source message contains an enum value the recipient stubs do not understand.
For instance if a message is sent where the enum value is specified to be 3, this example would then report UNRECOGNIZED

proto definition:

enum EnumStatus {
  ENUM_STATUS_UNSPECIFIED = 0;
  ENUM_STATUS_STARTED = 1;
  ENUM_STATUS_STOPPED = 2;
}

Generated java stub:

  /**
   * Protobuf enum {@code EnumStatus}
   */
  public enum EnumStatus
      implements com.google.protobuf.ProtocolMessageEnum {
    /**
     * <code>ENUM_STATUS_UNSPECIFIED = 0;</code>
     */
    ENUM_STATUS_UNSPECIFIED(0),
    /**
     * <code>ENUM_STATUS_STARTED = 1;</code>
     */
    ENUM_STATUS_STARTED(1),
    /**
     * <code>ENUM_STATUS_STOPPED = 2;</code>
     */
    ENUM_STATUS_STOPPED(2),
    UNRECOGNIZED(-1),
    ;

    /**
     * <code>ENUM_STATUS_UNSPECIFIED = 0;</code>
     */
    public static final int ENUM_STATUS_UNSPECIFIED_VALUE = 0;
    /**
     * <code>ENUM_STATUS_STARTED = 1;</code>
     */
    public static final int ENUM_STATUS_STARTED_VALUE = 1;
    /**
     * <code>ENUM_STATUS_STOPPED = 2;</code>
     */
    public static final int ENUM_STATUS_STOPPED_VALUE = 2;


    public final int getNumber() {
      if (this == UNRECOGNIZED) {
        throw new java.lang.IllegalArgumentException(
            "Can't get the number of an unknown enum value.");
      }
      return value;
    }

    /**
     * @deprecated Use {@link #forNumber(int)} instead.
     */
    @java.lang.Deprecated
    public static EnumStatus valueOf(int value) {
      return forNumber(value);
    }

    public static EnumStatus forNumber(int value) {
      switch (value) {
        case 0: return ENUM_STATUS_UNSPECIFIED;
        case 1: return ENUM_STATUS_STARTED;
        case 2: return ENUM_STATUS_STOPPED;
        default: return null;
      }
    }

    public static com.google.protobuf.Internal.EnumLiteMap<EnumStatus>
        internalGetValueMap() {
      return internalValueMap;
    }
    private static final com.google.protobuf.Internal.EnumLiteMap<
        EnumStatus> internalValueMap =
          new com.google.protobuf.Internal.EnumLiteMap<EnumStatus>() {
            public EnumStatus findValueByNumber(int number) {
              return EnumStatus.forNumber(number);
            }
          };

    public final com.google.protobuf.Descriptors.EnumValueDescriptor
        getValueDescriptor() {
      return getDescriptor().getValues().get(ordinal());
    }
    public final com.google.protobuf.Descriptors.EnumDescriptor
        getDescriptorForType() {
      return getDescriptor();
    }
    public static final com.google.protobuf.Descriptors.EnumDescriptor
        getDescriptor() {
      return no.entur.mapstruct.example.protobuf.UserProtos.getDescriptor().getEnumTypes().get(0);
    }

    private static final EnumStatus[] VALUES = values();

    public static EnumStatus valueOf(
        com.google.protobuf.Descriptors.EnumValueDescriptor desc) {
      if (desc.getType() != getDescriptor()) {
        throw new java.lang.IllegalArgumentException(
          "EnumValueDescriptor is not for this type.");
      }
      if (desc.getIndex() == -1) {
        return UNRECOGNIZED;
      }
      return VALUES[desc.getIndex()];
    }

    private final int value;

    private EnumStatus(int value) {
      this.value = value;
    }

    // @@protoc_insertion_point(enum_scope:EnumStatus)
  }

Generated enum mapping:

    @Override
    public Status map(EnumStatus permissionDTO) {
        if ( permissionDTO == null ) {
            return null;
        }

        Status status;

        switch ( permissionDTO ) {
            case ENUM_STATUS_UNSPECIFIED: status = null;
            break;
            case ENUM_STATUS_STARTED: status = Status.STARTED;
            break;
            case ENUM_STATUS_STOPPED: status = Status.STOPPED;
            break;
            case UNRECOGNIZED: status = null;
            break;
            default: throw new IllegalArgumentException( "Unexpected enum constant: " + permissionDTO );
        }

        return status;
    }

    @Override
    public EnumStatus map(Status perm) {
        if ( perm == null ) {                          // Hidden by the NullValueCheckStrategy
            return EnumStatus.UNRECOGNIZED;
        }

        EnumStatus enumStatus;

        switch ( perm ) {
            case STARTED: enumStatus = EnumStatus.ENUM_STATUS_STARTED;
            break;
            case STOPPED: enumStatus = EnumStatus.ENUM_STATUS_STOPPED;
            break;
            default: throw new IllegalArgumentException( "Unexpected enum constant: " + perm );
        }

        return enumStatus;
    }

NullValueCheckStrategy

Not following here. If you defined @ValueMapping with source=MappingConstants.NULL you are explicitly defining the mapping when source is null. It should not apply the one from the strategy. I think I added tests for that

The mapper definiton:
@Mapper(collectionMappingStrategy = CollectionMappingStrategy.ADDER_PREFERRED, nullValueCheckStrategy = NullValueCheckStrategy.ALWAYS, unmappedSourcePolicy = ReportingPolicy.ERROR, unmappedTargetPolicy = ReportingPolicy.ERROR) public interface UserMapper {

The code generated code :

        if ( user.getStatus() != null ) {               // Null check here hides value mapping from null to EnumStatus.UNRECOGNIZED
            userDTO.setStatus( map( user.getStatus() ) );
        }

Please see the comment // Hidden by the NullValueCheckStrategy - yes it was not explicitly defined but returned by the SPI implementation as the mapped value for the null enum value.

I think that now I understand what you mean. You expect the mapping from null Status to EnumStatus to return EnumStatus.UNRECOGNIZED. However, with this PR it returns EnumStatus.ENUM_STATUS_UNSPECIFIED instead.

With this PR:

    @Override
    public EnumStatus map(Status perm) {
        if ( perm == null ) {
            return EnumStatus.ENUM_STATUS_UNSPECIFIED;
        }
        //...
    }

Expected:

    @Override
    public EnumStatus map(Status perm) {
        if ( perm == null ) {
            return EnumStatus.UNRECOGNIZED;
        }
        //...
    }

Am I on the right track? Does this branch from your example generate 100% what you expect?

I tried to be a bit smart and automatically detect the mapping from null to the default value, but indeed it seems like it is not enough.

Regarding the NullValueCheckStrategy. That has nothing to do with the enum mappings. With it you are basically telling MapStruct to only invoke mappings when the source is not null, you will need to make sure that your defaults are properly set in your target objects or not use NullValueCheckStrategy.ALWAYS

@filiphr
I want it to return the value I define in a method similar to
https://github.com/entur/mapstruct-spi-protobuf/blob/d79ff5c878d2fc32f2253a802fb5166b36934ccd/spi-impl/src/main/java/no/entur/mapstruct/example/protobuf/ProtobufEnumNamingStrategy.java#L51

Your current PR generates the following (on my computer, I have not checked how you select which EnumStatus constant to generate - maybe indeterministic?)

    @Override
    public EnumStatus map(Status perm) {
        if ( perm == null ) {
            return EnumStatus.UNRECOGNIZED; // Invalid value, blows up runtime
        }
        //...
    }

but I do want this

 @Override
    public EnumStatus map(Status perm) {
        if ( perm == null ) {
            return EnumStatus.ENUM_STATUS_UNSPECIFIED; // I want to specify this using getDefaultEnumConstant or similar
        }
        //...
    }

which is the opposite of what you wrote (you probably copy pasted wrongly?)

I pushed my updated branch based on this PR:
https://github.com/entur/mapstruct-spi-protobuf/blob/d79ff5c878d2fc32f2253a802fb5166b36934ccd/spi-impl/src/main/java/no/entur/mapstruct/example/protobuf/ProtobufEnumNamingStrategy.java

The code here https://github.com/entur/mapstruct-spi-protobuf/blob/valuemapping_accessor_spi/spi-impl/src/main/java/no/entur/mapstruct/example/protobuf/ProtobufEnumConstantNamingStrategy.java
generates 100% correct code using #2045

Ref NullValueCheckStrategy ok, I understand that they solve different aspects. I might be able to solve it differently then. Protobuf stubs are littered with throws NullPointerException in case one tries to set a property to null

Thanks a lot for the patience @seime. I understand it now. I wanted to avoid the public String getDefaultEnumConstant(TypeElement enumType), by being a bit smart with the special <NULL> constant.

I will remove this smartness and add a method for that. However, I'll keep the special <NULL> constant to be able to map into null.

@seime I've updated this PR by adding the new method. I think that now it covers your use case 100%

Thanks a lot for the review @sjaakd.

I think you need to update the specifications.. Also, I'm not entirely sure whether this is the proper way of handling. I can imagine that sometimes you don't want to apply the strategy and now its suddenly always there.

Yes I'll update the documentation and I'll try to make it clearer. This strategy passes the enum you are trying to map so I see it similar as an AccessorNamingStrategy. Perhaps a company policy with certain way of mapping certain enums. You have access to the entire TypeElement so you can check packages, interfaces etc. on it.

For instance,, suppose you would apply this strategy to your organisation. Would I expect, working in a team totally not related to a team working on protobuf functionality this kind of behaviour? I'm still not sure whether SPI is a proper way forward.

That's why the protobuf functionality will only be applied if the enum is a protobuf enum. Btw we don't provide out of the box support for this. On top of it you can still use the original names of the enums in @ValueMapping. This is only applied for implicit mapping

@sjaakd, I've updated the documentation. Does it make more sense now? I removed the mention of Protobuf in order to keep it simpler.

Hey Filip.. I think its ok.. I understand that the strategy allows for spotting certain enum types and based on the type adapt the enum name. It's ok. Although I think that the use cases is (in the end) very protobuf specific (I wonder whether there would be even other cases, but I'd like to be proven wrong). .. I guess the protobuf one would end up somewhere in our examples or even as a pre-delivered SPI?.

@sjaakd, I've updated the documentation. Does it make more sense now? I removed the mention of Protobuf in order to keep it simpler.

Hey Filip.. did you push your changes?

Although I think that the use cases is (in the end) very protobuf specific (I wonder whether there would be even other cases, but I'd like to be proven wrong). .. I guess the protobuf one would end up somewhere in our examples or even as a pre-delivered SPI?.

Yes the use case is very protobuf specific right now. Let's see what the future brings, there might be some other use cases as well. Remember @Context 😉. Not sure whether a pre-delivered SPI or more like an extension, let's see after the release.

Hey Filip.. did you push your changes?

Seems like I forgot 🤦. Just pushed now.

@seime I've updated this PR by adding the new method. I think that now it covers your use case 100%

@filiphr, I am on holiday atm but I will take a look Monday morning!

Thanks,
Arne

@filiphr LGTM!

Thanks a lot for the review @seime