Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Paged Responses return null for api cals using rest-java client #2588

Closed
rpopa opened this Issue Mar 1, 2019 · 5 comments

Comments

3 participants
@rpopa
Copy link

rpopa commented Mar 1, 2019

When I make a call using the rest-java-client to one of the apis that contain a paged response, the response status is 200(OK) but the content is null.

Calling the api getProcessInstaces from the postman I receive this response:

{
 "list": {
 "entries": [
 {
 "entry": {
 "appName": "default-app",
 "appVersion": "",
 "serviceName": "rb-my-app",
 "serviceFullName": "rb-my-app",
 "serviceType": "runtime-bundle",
 "serviceVersion": "",
 "id": "03e1d986-39fe-11e9-a789-4e8455e68fba",
 "startDate": "2019-02-26T19:37:56.111+0000",
 "initiator": "admin",
 "status": "RUNNING",
 "processDefinitionId": "SimpleProcess:1:064909c0-39e3-11e9-a789-4e8455e68fba",
 "processDefinitionKey": "SimpleProcess",
 "processDefinitionVersion": 1
 }
 }, ....

but when I do that using the generated client based on swagger definition, the response in 200 but the body is empty. After some debugging, the response is received ok but it is lost after decoding.

The PagedResourcesProcessInstanceResource response does not seem to match the response I've got from postman.

@erdemedeiros

This comment has been minimized.

Copy link
Member

erdemedeiros commented Mar 6, 2019

After some investigation with @constantin-ciobotaru the problem seems to come from the generated Swagger file that's referencing HAL classes, note that in the snippets bellow the schema is referencing ProcessInstanceResource which is a class used to produce HAL response. However when application/json is chosen as mediaType, AlfrescoJackson2HttpMessageConverter will convert PagedResources«ProcessInstanceResource» to AlfrescoPageContentListWrapper<CloudProcessInstance> producing a response that's not matching with client classes structure.

We need somehow tell swagger api generator to replace PagedResources«ProcessInstanceResource»by AlfrescoPageContentListWrapper<CloudProcessInstance>.

{
  "/v1/process-instances": {
    "get": {
      "tags": [
        "process-instance-controller-impl"
      ],
      "summary": "getProcessInstances",
      "operationId": "getProcessInstancesUsingGET_1",
      "produces": [
        "application/json",
        "application/hal+json"
      ],
      "responses": {
        "200": {
          "description": "OK",
          "schema": {
            "$ref": "#/definitions/PagedResources«ProcessInstanceResource»"
          }
        },
        "401": {
          "description": "Unauthorized"
        },
        "403": {
          "description": "Forbidden"
        },
        "404": {
          "description": "Not Found"
        }
      }
    }
  }
}
{
  "PagedResources«ProcessInstanceResource»": {
    "type": "object",
    "properties": {
      "content": {
        "type": "array",
        "items": {
          "$ref": "#/definitions/ProcessInstanceResource"
        }
      },
      "links": {
        "type": "array",
        "xml": {
          "name": "link",
          "namespace": "http://www.w3.org/2005/Atom",
          "attribute": false,
          "wrapped": false
        },
        "items": {
          "$ref": "#/definitions/Link"
        }
      },
      "page": {
        "$ref": "#/definitions/PageMetadata"
      }
    }
  }
}

@salaboy salaboy added this to the 7.1.0.M1 milestone Mar 6, 2019

@salaboy salaboy added this to Open in Activiti 7.x via automation Mar 6, 2019

@salaboy salaboy added the priority1 label Mar 6, 2019

@erdemedeiros

This comment has been minimized.

Copy link
Member

erdemedeiros commented Mar 11, 2019

After further investigation it turns out that the client generation for HAL format is not working neither. A couple of problems:

  • Generated client is considering application/json and application/hal+json as a single media type. I.e. the generated api has only produces = "application/json" where it should be produces = {"application/json", "application/hal+json"} (as per swagger specification file).
    Note: if media types are from different formats (i.e. application/json and application/xml) the client api mentions both properly, the problem only happens for variants the the same format.
  • After manually updating the generated client to produce application/hal+json instead of application/json the client is still broken. The problem happens because Swagger Codegen does not reuse Spring classes such as ResourceSupport, Resources and PagedResources, but it generates new flatten classes for each EntityType (ProcessDefinitionResource, PagedResourcesProcessDefinitionResource, etc). See swagger-api/swagger-codegen#7081 and swagger-api/swagger-codegen#7193. However, the ObjectMapper in charge of deserialisation will not be aware of some customisations registered via Mixins only for Spring classes.

I got the client eventually working after some extra changes:

  • Create a dedicated Mixin class
@JsonPropertyOrder({ "content", "links" })
public abstract class AlfrescoResourcesMixin<T> extends Resources<T> {

    @Override
    @JsonProperty("_embedded")
    @JsonInclude(JsonInclude.Include.NON_EMPTY)
    @JsonSerialize(using = Jackson2HalModule.HalResourcesSerializer.class)
    @JsonDeserialize(using = Jackson2HalModule.HalResourcesDeserializer.class)
    public abstract Collection<T> getContent();

    @JsonProperty("_links")
    @JsonInclude(JsonInclude.Include.NON_EMPTY)
    @JsonSerialize(using = Jackson2HalModule.HalLinkListSerializer.class)
    @JsonDeserialize(using = Jackson2HalModule.HalLinkListDeserializer.class)
    public abstract List<Link> getLinks();

}
  • Register Mixins for every generated resource I wanted to cover via a custom Module:
public class AlfrescoJackson2HalModule extends Jackson2HalModule {

    public AlfrescoJackson2HalModule() {
        super();
        setMixInAnnotation(ProcessDefinitionResource.class, AlfrescoResourcesMixin.class);
        setMixInAnnotation(PagedResourcesProcessDefinitionResource.class, AlfrescoResourcesMixin.class);

    }
  • Create a custom decoder:
public class HalDecoder extends ResponseEntityDecoder {

    public HalDecoder(ObjectMapper objectMapper) {
        super(new JacksonDecoder(objectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
                                         .registerModule(new AlfrescoJackson2HalModule())));
    }
}
  • Register the custom decoder:
    @Bean
    public Decoder feignDecoder(ObjectMapper objectMapper) {
      return new HalDecoder(objectMapper);
    }

Even if it's working it doesn't seem to be the way to go as it would require the registration of Mixins for every resource type in the api.

Notes about swagger-codegen-maven-plugin configuration:

  • version: 2.3.1
  • language + library: spring / spring-cloud (with custom mustache templates)
<language>spring</language>
<library>spring-cloud</library>
<templateDirectory>${project.basedir}/src/main/resources/templates</templateDirectory>
@salaboy

This comment has been minimized.

Copy link
Member

salaboy commented Mar 11, 2019

@erdemedeiros erdemedeiros self-assigned this Mar 15, 2019

@erdemedeiros erdemedeiros moved this from Open to In Progress in Activiti 7.x Mar 15, 2019

erdemedeiros added a commit to Activiti/activiti-cloud-runtime-bundle-service that referenced this issue Mar 15, 2019

Fix swagger specification for Alfresco MediaType
Remove [DomainObject]Resource classes as they are not bringing any value and this allows the reuse of common swagger
configuration defined in common repository.

Refs Activiti/Activiti#2588

erdemedeiros added a commit to Activiti/activiti-cloud-service-common that referenced this issue Mar 15, 2019

erdemedeiros added a commit to Activiti/activiti-cloud-service-common that referenced this issue Mar 15, 2019

erdemedeiros added a commit to Activiti/activiti-cloud-query-service that referenced this issue Mar 18, 2019

Fix swagger specification for Alfresco MediaType
Remove [DomainObject]Resource classes as they are not bringing any value and this allows the reuse of common swagger
configuration defined in common repository.

Refs Activiti/Activiti#2588

erdemedeiros added a commit to Activiti/activiti-cloud-audit-service that referenced this issue Mar 18, 2019

Fix swagger specification for Alfresco MediaType
Remove [DomainObject]Resource classes as they are not bringing any value and this allows the reuse of common swagger
configuration defined in common repository.

Refs Activiti/Activiti#2588

ryandawsonuk added a commit to Activiti/activiti-cloud-runtime-bundle-service that referenced this issue Mar 18, 2019

Fix swagger specification for Alfresco MediaType (#270)
Remove [DomainObject]Resource classes as they are not bringing any value and this allows the reuse of common swagger
configuration defined in common repository.

Refs Activiti/Activiti#2588

ryandawsonuk added a commit to Activiti/activiti-cloud-query-service that referenced this issue Mar 18, 2019

Fix swagger specification for Alfresco MediaType (#209)
Remove [DomainObject]Resource classes as they are not bringing any value and this allows the reuse of common swagger
configuration defined in common repository.

Refs Activiti/Activiti#2588

ryandawsonuk added a commit to Activiti/activiti-cloud-audit-service that referenced this issue Mar 18, 2019

Fix swagger specification for Alfresco MediaType (#160)
Remove [DomainObject]Resource classes as they are not bringing any value and this allows the reuse of common swagger
configuration defined in common repository.

Refs Activiti/Activiti#2588
@erdemedeiros

This comment has been minimized.

Copy link
Member

erdemedeiros commented Mar 18, 2019

Note: problem solved for alfresco format by using alternateTypeRules and by making controllers return PagedResources<Resource<DomainObject>>, Resources<Resource<DomainObject>>, Resource<DomainObject> instead of PagedResources<DomainObjectResource>, Resources<DomainObjectResource>, Resource<DomainObject>, respectively. I.e. https://github.com/Activiti/activiti-cloud-runtime-bundle-service/pull/270/files#diff-db3c2ee686283b318bc6cd244c8aec0eL16

I'll update notes about HAL format soon.

@salaboy, for information, I'm reaching this bug when trying version 3.0.5 of swagger-codegen-maven-plugin. However the problem seems to be more at swagger file specification generation than client code generation from swagger file.

@salaboy

This comment has been minimized.

Copy link
Member

salaboy commented Mar 18, 2019

@erdemeiros that is usually not a problem.. that means that the @FeignClient has that url into the service URL configuration. That can be solved by using variables to specify the protocol://host:port to make sure that swagger codegen knows where to get access to the service.
Can you share the class that has "/api/v1/..."? Notice that this is from the client side not in the server side. It doesn't really matter what the @RestController says..

erdemedeiros added a commit to Activiti/activiti-cloud-acceptance-tests that referenced this issue Mar 20, 2019

erdemedeiros added a commit to Activiti/activiti-cloud-acceptance-scenarios that referenced this issue Mar 20, 2019

erdemedeiros added a commit to Activiti/activiti-cloud-org-service that referenced this issue Mar 20, 2019

Fix swagger specification for Alfresco MediaType
Reuse of common swagger configuration defined in common repository.

Refs Activiti/Activiti#2588

erdemedeiros added a commit to Activiti/activiti-cloud-service-common that referenced this issue Mar 20, 2019

erdemedeiros added a commit to Activiti/activiti-cloud-service-common that referenced this issue Mar 20, 2019

mergify bot added a commit to Activiti/activiti-cloud-org-service that referenced this issue Mar 21, 2019

Fix swagger specification for Alfresco MediaType (#193)
* Fix swagger specification for Alfresco MediaType

Reuse of common swagger configuration defined in common repository.

Refs Activiti/Activiti#2588

* Inject ApiInfo as a bean

mergify bot added a commit to Activiti/activiti-cloud-acceptance-tests that referenced this issue Mar 25, 2019

Add Swagger api endpoints (#144)
* Add Swagger api endpoints

Refs Activiti/Activiti#2588

* Add steps for modeling

miguelruizdev added a commit to Activiti/activiti-cloud-acceptance-scenarios that referenced this issue Mar 25, 2019

mergify bot added a commit to Activiti/activiti-cloud-acceptance-scenarios that referenced this issue Mar 25, 2019

Add scenario to retrieve Swagger specification (#251)
* Add scenario to retrieve Swagger specification

Refs Activiti/Activiti#2588

* Add scenario for modeling

* Fix typo

Activiti 7.x automation moved this from In Progress to Closed Mar 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.