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

SelectedValueComparator triggering a lot of database lookups with DomainClassConverter [SPR-12472] #17078

Closed
spring-issuemaster opened this issue Nov 26, 2014 · 18 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Nov 26, 2014

Stephan Sundermann opened SPR-12472 and commented

When using select tags with spring form binding, prefilling these selects in the form object will trigger a lot of database lookups when a DomainClassConvert is used leading to massive performance issues.
The issue seems to happen in SelectedValueComperator which has a lot of comparisons and continues to go down through the comparisons until one comparison returns true. Eventually it will hit exhaustiveCompare() which will then trigger the conversion to a domain object. When there are for example 100 options in the select tag it will trigger in the worst case 100 database lookups.

In my form object I have:

public class NewCourseForm {
...
    @NotEmpty(message = NOT_BLANK_MESSAGE)
    private List<Lecturer> lecturers;
...
}

The domain class:

@Entity
@Table(name="Lecturer")
@Data
public class Lecturer implements Serializable {
...
    @Override
    public boolean equals(Object lecturer) {
        if(lecturer instanceof Integer) {
            return this.id == lecturer;
        } else if(lecturer instanceof String) {
	     return Integer.toString(this.id).equals(lecturer);
	} else if(lecturer instanceof Lecturer) {
            return this.id == (((Lecturer) lecturer).getId());
        } else return false;
    }
...

My fix for now was to disable the exhaustiveCompare, but this seems to be a bad way and can probably not get merged. Is there something I am missing here? Is there an annotation or interface which I can implement to tell spring to just use my equals method I implemented in my domain object without doing a exhaustiveCompare?


Affects: 4.1.2, 4.1.3

Attachments:

Issue Links:

  • #14089 Slow JSP render for OptionTag ("is duplicated by")
  • DATACMNS-672 Entity is converted to its ID unexpectedly in a Spring EL expression

Referenced from: commits spring-projects/spring-framework-issues@8788e6c, spring-projects/spring-framework-issues@49670a6

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 27, 2014

Brian Clozel commented

Hi Stephan Sundermann

I'm trying to reproduce exactly your use case, could you copy/paste here the method(s) signature(s) of your controllers and what you're putting in the Model?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 27, 2014

Stephan Sundermann commented

Hi,

alright. The first thing I have is a DomainClassConverter in my WebappInitializer:

@Bean
public DomainClassConverter<?> domainClassConverter() {
    return new DomainClassConverter<>(mvcConversionService());
}

In my controller I populate my model:

    @RequestMapping(value = "/new/fromAbstract")
    public String newCourseFromAbstract(AbstractCourse abstractCourse, Semester semester, Model model) {
..
                model.addAttribute("allLecturers", lecturerService.getLecturers()); // All possible values for the select options, which is just a list of my domain objects from hibernate
...
        // Instantiate a new form object
        NewCourseForm courseForm = new NewCourseForm();
        // Set the selected options
        courseForm.setLecturers(abstractCourse.getLecturers());
...
        return "course/new"; // After the return all the conversions happen in the template engine because exhaustiveCompare is triggered for almost each option.
}

For the view I am using Thymeleaf with the following markup binding the select's value to the List<Lecturers> in my form object.

<select id="lecturers" multiple="true" th:field="*{lecturers}">
    <option th:each="lecturer : ${allLecturers}" th:text="${lecturer}" th:value="${lecturer.id}">Lecturer</option>
</select>

The effect becomes visible when having a lot of options in the model. Turn on hibernate's query logging to see all the database lookups getting triggered.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 27, 2014

Brian Clozel commented

Thanks!

This is a rather tricky problem, involving a lot of pieces and projects (thymeleaf or JSP + spring + spring-data-commons).
What do you think about doing this - would that require a lot of extra work?

public class NewCourseForm {
...
    @NotEmpty(message = NOT_BLANK_MESSAGE)
    private List<Integer> lecturersIds;
...
}
<select id="lecturers" multiple="true" th:field="*{lecturersIds}">
    <option th:each="lecturer : ${allLecturers}" th:text="${lecturer}" th:value="${lecturer.id}">Lecturer</option>
</select>

Not suggesting this as a workaround or a solution, but just wanted your opinion - I'm trying to get the big picture here.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 27, 2014

Stephan Sundermann commented

I actually tried making the object in the form a list of integers but for some reason I got the same amount of conversions being triggered.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 27, 2014

Brian Clozel commented

This is trying to convert from a String (actually the id in the String form) to a Lecturer instance.
To do that, you can register a custom converter or rely on convention (see this blog post).

So adding a valueOf method in the Lecturer class might help here.

public class Lecturer {
  // the conversion algo will use this
  public static Lecturer valueOf(String id) { ... }
}

I got it working in a repro project. This may not be the best solution here, but in my project the DomainClassConverter was not triggered with this.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 27, 2014

Stephan Sundermann commented

In the valueOf method I would create a new Lecturer object and set the id? I guess I can live with that fix.

EDIT: I haven't tested it yet, but how does Spring determine whether to do the conversion using the DomainClassConveter or the valueOf method? In my controllers I want to have automatic conversion from the String to the entity object, but I don't want that conversion when Spring tries to evaluate which option to select in the template. It seems that this fix would also disable automatic conversion everywhere not just in the SelectValueComperator.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 27, 2014

Brian Clozel commented

Right. Let me test this in the repro example and get back to you.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 27, 2014

Brian Clozel commented

Stephan Sundermann - this is basically a brain dump, don't hesitate to correct me if misrepresented the actual issue. I'm working on workarounds and actual solutions to get this sorted out.

The actual issue

I've added a repro project as it's not easy to picture the whole thing.

When using <select> tags in JSPs or Thymeleaf, tags implementations call org.springframework.web.servlet.tags.form.SelectedValueComparator to check as "selected" values that are contained in the list in the form backing object.

In the repro project:

@RequestMapping("/")
	public String home(Model model) {

		model.addAttribute("userList", repository.findAll());
		logger.info("fetched the list of all users");

		UserForm form = new UserForm() ;
		form.setSelectedUsers(Arrays.asList(repository.findOne(9L), repository.findOne(10L)));
		logger.info("selected two particular users");

		model.addAttribute("userForm", form);

		return "index";
	}
<form action="#" th:action="@{/test}" th:object="${userForm}" method="post">
  <!-- backing this part of the form with the list of selected users in userForm.getSelectedUsers() -->
  <select id="users" multiple="true" th:field="*{selectedUsers}">
    <!-- writing an option tag per item in the list of all users -->
    <option th:each="user : ${userList}" th:text="${user.name}" th:value="${user.id}">User</option>
  </select>
</form>

Problems happen when the following conditions are met:

  1. Use of the DomainClassConverter, a Converter automatically added by spring-data-commons when using the annotation @EnableSpringDataWebSupport; this converter converts simple types to domain types by fetching data in the datastore. This is really useful to convert path parameters to domain objects like public String showuser(@PathVariable("id") User user)
  2. Use of <select> tags in templating engines that make use of the SelectedValueComparator, such as Spring's JSP support or Thymeleaf
  3. We're trying to compare a basic type to a domain object in the template

What happens is that SelectedValueComparator has a quite agressive algorithm, since it's using several methods to make sure instances are truly equal or not: using .equals, then propertyEditors, converters, toString, etc... So even if .equals says that object aren't equal, the algorithm continues with SelectedValueComparator.exhaustiveCompare which in this case triggers the DomainClassConverter within the template, thus making multiple database calls (maximum number of selected items * number of items in the list).

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Nov 27, 2014

Stephan Sundermann commented

That's exactly what I am experiencing!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Dec 7, 2014

Stephan Sundermann commented

Any progress on this?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jan 5, 2015

Oliver Drotbohm commented

There's quite a few things one can do. First of all I'd recommend using th:selected="…" to manually define the criteria on when an item is considered selected. This will effectively avoid the exhaustive comparison to be kicked off at all.

Changing the equals(…) method as you showed up there is not a good idea as the implementation is violating the equals-contract.

For DATACMNS-627 I've changed the DomainClassConverter to also be able to translate the entity back into its identifier type and transitively convertible types. That's basically our part in an improvement that can be made in SelectedValueComparator. exhaustiveCompare(…)'s first step is to create a String representation of the candidate value. With the change for DATACMNS-627 in place, this should translate into the String representation of the entities identifier. If now the second step (creating a to-string-representation of the bound value) is changed to also use the conversion mechanism, this will create identifier values as Strings, too and thus should skip the expensive lookups at least for the values actually selected.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jan 6, 2015

Brian Clozel commented

Hi Stephan Sundermann

I've updated the repro project with a couple of possible fixes/workaround.

I'd say this issue is triggered by using Spring Data's DomainClassConverter and Thymeleaf templating syntax for select tags (which in turn uses Spring's Comparator).

The thing is, we're pretty much stuck here trying to convert a candidate value (the user id) to a User domain type. Even if we make changes to this Comparator (like having a nonExhaustiveCompare), Thymeleaf maintainers would have to update their codebase to reflect that change (also, note that this Comparator was initially designed for JSPs).

Could you maybe open an issue in the Thymeleaf project, explain this situation (and point them here maybe)? This probably has more consequences for them than we think, such as the current syntax, expected behavior, etc.

I'd be happy to assist and improve Spring to better support those cases.

Thanks

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jan 14, 2015

Daniel Fernández commented

Hi guys,

Stephan created this ticket thymeleaf/thymeleaf#341 on the thymeleaf issue system for covering this topic.

As I mention there, Thymeleaf's Spring "th:field" integration tries to mirror what the Spring JSP tag libraries do, so that people migrating applications from JSP to thymeleaf -- we hope the contrary would never happen ;) -- don't suffer any unpleasant surprises.

In this case, Thymeleaf uses SelectedValueComparator because Spring's OptionTag does (see https://github.com/spring-projects/spring-framework/blob/master/spring-webmvc/src/main/java/org/springframework/web/servlet/tags/form/OptionTag.java#L237-L239 ), and I would only feel really comfortable changing this behaviour in Thymeleaf if you have plans for changing the way this is done in JSP too, so that we can keep on mirroring Spring JSP taglib's behaviour here...

Are you planning to do any changes in this regard?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jan 15, 2015

Brian Clozel commented

Hey Daniel Fernández

I totally understand the constraints here. On my side, I'm a bit reluctant to change that behavior in our JSP support (backwards compatibility, behavior, etc); adding a new parameter to disable this "exhaustive comparison" won't really be useful IMO, since you've got to figure all this first...

There was another possibility:

  • using th:value="${user}" instead of th:value="${user.id}"
  • rely on Spring converters (here, the DomainClassConverter) to convert the domain object to String for printing the value, instead of calling toString directly
  • equality would then happen between domain objects, not String representations

But this requires non-trivial changes, behavior changes (and performance issues?) in Thymeleaf.
Besides what I've suggested in my previous comment, I don't have a more clever solution for this - and this precise use case may not be that ubiquitous - but I'm still open for discussion if you've got improvement ideas in that space...

Thanks,

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jan 18, 2015

Stephan Sundermann commented

Can we not have something like:

@DisableExhaustiveCompare
public boolean equals(Object obj) {...}

In general the documentation is not straightforward in terms of what it is exaclty doing when trying to select options. For thymeleaf, all I found is that an equals Implementation which will be used to determine the selected options has to be provided. Exhaustive compare is not documented anywhere I found about it when reading the code and trying to fix the issue I was having.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jan 18, 2015

Daniel Fernández commented

Actually, using th:value for forcing a new kind of object equality based on the execution of the conversion service would not require any kind of changes in Thymeleaf, if that's what you mean.

Thymeleaf (2.1+) allows developers to apply the ConversionService whenever they want by using the double-brace syntax, as explained here: http://www.thymeleaf.org/doc/tutorials/2.1/thymeleafspring.html#the-conversion-service

So the conversion service can be applied manually at th:value, th:selected... wherever needed.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jan 18, 2015

Stephan Sundermann commented

So when using th:value="$domainObject" this wouldn't trigger the DomainClassConverter when spring tries to select the values?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jan 18, 2015

Daniel Fernández commented

The double-brace syntax will execute whichever Conversion Service you have configured at your Spring MVC infrastructure, Thymeleaf cannot reach further than that (it's not its responsibility). But this would allow using th:value in the way Brian suggests, if I understood correctly.

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