ScaCap / spring-auto-restdocs

Spring Auto REST Docs is an extension to Spring REST Docs
https://scacap.github.io/spring-auto-restdocs/
Apache License 2.0
311 stars 86 forks source link

Enum constraint message is not appended to description field when wrapped in an Array #423

Closed mustaphazorgati closed 3 years ago

mustaphazorgati commented 4 years ago

Hey!

When you want to allow an aggregation of any request parameter spring allows two ways:

When using a list the type is detected as Object and the Enum constraint is added to the description. When using an array the type is Array[Object] and the Enum constraint is not added to the description.

This forces us to always use the list which works, but the code does not look nice since we have to convert that list to an array for further processing.

I'd love it if this can be fixed. I am happy to help out if necessary.

mustaphazorgati commented 4 years ago

This is using Java 11.0.8 ;)

mustaphazorgati commented 3 years ago

Update: this also applies for the RequestFieldSnippet. There the type is Array[String] but the Enum constraint is only added when using List as data type.

jmisur commented 3 years ago

Thanks for reporting. Have you tried to debug the code to pinpoint the fix? I see you already dig into the sources.

mustaphazorgati commented 3 years ago

Not yet. I can do a timeboxed debugging session.

mustaphazorgati commented 3 years ago

I tried to differentiate between the multiple issues here:

  1. Appending the Enum constraint message in the RequestFieldSnippet for Array[Enum]
  2. Appending the Enum constraint message in the RequestParametersSnippet for Array[Enum]
  3. Properly detecting the Enum type in RequestParametersSnippet for List and Array[Enum]

Regarding 1. I found the issue in ConstraintReaderImpl#getEnumConstraintMessage(Class<?>, String) That method only checks if the given field type is an Enum. If not the first generic type is fetched. I think the assumption here was that nested types only exists through generics. Arrays are ignored.
Solution: verify that the type is an array before fetching the first generic type. Entire Method regarding arrays as a possible type:

  private List<String> getEnumConstraintMessage(Class<?> javaBaseClass, String javaFieldName) {
        // could be getter actually
        Field field = findField(javaBaseClass, javaFieldName);
        if (field == null) {
            return emptyList();
        }

        if (field.getType().isEnum()) {
            return getEnumConstraintMessage(field.getType());
        } else if (field.getType().isArray()) {
            return getEnumConstraintMessage(field.getType().getComponentType());
        } else {
            return getEnumConstraintMessage(firstGenericType(field.getGenericType(), javaBaseClass));
        }
    }

This change is non-breaking (all existing tests pass). Since I only scratched the surface of your framework I ask you to take over here and add tests / examples where needed. It would take way too long for me to find out what I have to add where. Since we both live in Germany I can offer you a remote pair programming session. Maybe over the weekend if you're interested. This would allow me to get further insights into the project and maybe keep contributing in the future.

Meanwhile I'll continue investigating 2. and 3.

mustaphazorgati commented 3 years ago

I found the Issue regarding 2. Like 1) the treatment of an array type was omitted in ConstraintReaderImpl#getEnumConstraintMessage(MethodParameter) Updated Method (with array treatment):

private List<String> getEnumConstraintMessage(MethodParameter param) {
        if (param.getParameterType().isEnum()) {
            return getEnumConstraintMessage(param.getParameterType());
        } else if (param.getParameterType().isArray()) {
            return getEnumConstraintMessage(param.getParameterType().getComponentType());
        } else {
            return getEnumConstraintMessage(firstGenericType(param));
        }
    }
  1. is up next ;)
mustaphazorgati commented 3 years ago

Found the issue for 3: AbstractParameterSnippet#addFieldDescriptor([too many arguments..]): String parameterTypeName = determineTypeName(param.nestedIfOptional().getNestedParameterType()); You generate the parameter type name in TypeUtil#determineTypeName using the canoncical name. For anything that implents the Collection interface the canonical name will result to Object since we lose all the information we can retrieve using reflection. This explains why Collection<Enum> parameter will be resolved to Object type and a Array[Enum] parameter to Array[Object]. I have a rough idea how to solve this (should not be complicated). As mentioned above I'd prefer to either pass this to you or work together with you.

jmisur commented 3 years ago

Thanks for fixing it for arrays. Can you create a draft PR for collections if you want?

mustaphazorgati commented 3 years ago

Hey @jmisur! Sorry for the late response. I was sick.

I don't see why this should not work on Collections.

jmisur commented 3 years ago

I guess this is fixed then.