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

Cycles in Java classes cause infinite loop in ModelAttributeParameterExpander #621

Closed
candrews opened this issue Mar 23, 2015 · 16 comments
Closed
Labels
Milestone

Comments

@candrews
Copy link

When using JPA, Hibernate, and other ORMs, it's common for each side of a relationship to refer to the other. For example:

class Person {
    private String firstName;
    private String lastName;
    private Address address;

    public String getFirstName() {
        return firstName;
    }

    public void setFirstName(String firstName) {
        this.firstName = firstName;
    }

    public String getLastName() {
        return lastName;
    }

    public void setLastName(String lastName) {
        this.lastName = lastName;
    }
    public Address getAddress() {
        return address;
    }

    public void setAddress(Address address) {
        this.address= address;
    }
}

class Address {
    private String street;
    @ApiModelProperty(hidden = true)
    private Person person;

    public String getStreet() {
        return street;
    }

    public void setStreet(String street) {
        this.street = street;
    }
    public Person getPerson() {
        return person;
    }

    public void setPerson(person person) {
        this.person= person;
    }
}

Currently, this set of classes causes an infinite loop resulting in a StackOverflowError in com.mangofactory.swagger.readers.operation.parameter.ModelAttributeParameterExpander.expand(String, Class<?>, List). The problem is that the Java classes are expanded but the annotation which would cause the field to be excluded is being processed, so Person references Address references Person... and the expansion continues infinitely.

I think the solution is to simply process @ApiModelProperty(hidden = true) annotations at expansion time. It may also be nice to detect when such cycles occur and throw a descriptive error (instead of getting a StackOverflowError) but that's just a nice to have... solving the problem would quite sufficient :-)

Thanks!

@dilipkrish
Copy link
Member

@candrews I suspect this may be a bug. Will take a look, but most likely its going to be fixed in 2.x

@candrews
Copy link
Author

The team here and I are very psyched about using Swagger, and this issue is a blocker for us, so I have to ask - what's the timeline for 2.x?

Also, my naive impression is that if a check for the @apimodel(hidden=true) annotation on the "field" variable in the "expand" method would solve this problem and doesn't seem like it could cause regressions, so is that solution workable, and would it be permitted in a near future non-2.x release?

@dilipkrish
Copy link
Member

Well its really close :) but i'll say at least 2 weeks before a public snapshot/release. Let me see if I can fix it in 1.0.2.

@dilipkrish
Copy link
Member

Having said that, I'd say a couple things

  • the new 2.0 the api will have changed a little bit (as regards SwaggerSpringMvcPlugin and is set up to support multiple service description formats, swagger 1.2 and swagger 2.0 are supported out of the box, with plans to support more formats on the way.
  • secondly all the swagger annotations will be supported across both those versions.

For a sneak peak take a look at the spike-to-seperate-models-and-dtos branch

@adrianbk
Copy link
Member

we shouldn't rely on an annotation as a means to break the cycle.

@dilipkrish
Copy link
Member

@candrews Out of curiosity why are these deep domain objects annotated with @ModelAttribute?

@candrews
Copy link
Author

I'm using them in controller methods, ex:

@Controller
public class AddressService {
    @Autowired
    private AddressService addressService;

    @ModelAttribute("address")
    public Address getAddress(@PathVariable int addressId) {
        return addressService.getById(addressId);
    }

    @RequestMapping(value = "/address/{addressId}", method = RequestMethod.POST)
    @ResponseBody
    public Address updateById(@PathVariable int addressId,
            @Valid @ModelAttribute("address") Address address) {
        return addressService.update(degree);
    }
}

There are a lot more methods (getting, updating, deleting, then using views/templates for html rendering, etc) but that's the basic idea. I think that's a pretty common approach. (Using view objects distinct from the entities seems to be a common alternative, but this pattern seems to work fine, too.)

@dilipkrish
Copy link
Member

@candrews It seems like its a lot harder to fix it that I had anticipated in 1.x, especially since I suspect you're expecting that Person is ignored in the Address object. Its a little simpler in 2.0 work we've been doing.

Unfortunately, I think it makes more sense for use to focus our efforts on getting 2.x out.

@dilipkrish dilipkrish added this to the 2.0 milestone Mar 23, 2015
@dilipkrish dilipkrish added the bug label Mar 24, 2015
@dilipkrish
Copy link
Member

This should now be fixed in 2.0

@cm325
Copy link

cm325 commented Jan 8, 2016

This doesn't appear to be fixed, I tried versions 2.0.1 - 2.1.1 and the latest 2.3.1 (which appears to have fixed the need for spring 4, yay!), the versions I could try that work with spring 3. I have the same setup, where entities reference one another, I applied the @ApiModelProperty(hidden = true) but I still get an endless stream of :

2016-01-08 12:47:52,601 [localhost-startStop-1] DEBUG springfox.documentation.spring.web.readers.parameter.ModelAttributeParameterExpander- Attempting to expand expandable field:
2016-01-08 12:47:52,601 [localhost-startStop-1] DEBUG springfox.documentation.spring.web.readers.parameter.ModelAttributeParameterExpander- Expanding parameter type:

ending in stackoverflow here:

springfox.documentation.spring.web.readers.parameter.ModelAttributeParameterExpander.expand(ModelAttributeParameterExpander.java:84)

@dilipkrish
Copy link
Member

@cm325 could you please try with 2.3.1 that is spring 3 compatible again

@dilipkrish
Copy link
Member

If not please open a 🐛

@cm325
Copy link

cm325 commented Jan 8, 2016

Hi, yes, sry, I just edited my comment to include the fact that I just saw that 2.3.1 is spring 3 compatible, and just tested with that as well, and unfortunately, the same error occurs. I will open new bug-

@dilipkrish
Copy link
Member

👍🏼 would be useful if u can provide a failing model or better still a
failing test.
On Fri, Jan 8, 2016 at 12:25 PM chris marx notifications@github.com wrote:

Hi, yes, sry, I just edited my comment to include the fact that I just saw
that 2.3.1 is spring 3 compatible, and just tested with that as well, and
unfortunately, the same error occurs. I will open new bug-


Reply to this email directly or view it on GitHub
#621 (comment)
.

@cm325
Copy link

cm325 commented Jan 8, 2016

I just posted this -

#1133

hoping that will be enough-

@yangshimeng
Copy link

help me .

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

No branches or pull requests

5 participants