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

Deeply nested (3+ levels) result map could cause IllegalArgumentException #1176

Closed
Rocky-Hu opened this Issue Jan 14, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@Rocky-Hu
Copy link

Rocky-Hu commented Jan 14, 2018

For some cases, when parse the mapper xml, it may be have incompleteStatements.
The Configuration#buildAllStatements will parse them latter.

But it maybe do twice.

The method org.apache.ibatis.binding.MapperMethod.SqlCommand#resolveMappedStatement:

if (configuration.hasStatement(statementId)) {
    return configuration.getMappedStatement(statementId);
}

call the hasStatement method, the validateIncompleteStatements is true, so it will call buildAllStatements method and add the statement to mappedStatements and return true.

then call getMappedStatement, the validateIncompleteStatements is also true, it will call buildAllStatements again.

public MappedStatement getMappedStatement(String id) {
    return this.getMappedStatement(id, true);
}
 
public MappedStatement getMappedStatement(String id, boolean validateIncompleteStatements) {
    if (validateIncompleteStatements) {
        buildAllStatements();
    }
    return mappedStatements.get(id);
}

it will throw 'java.lang.IllegalArgumentException' : Mapped Statements collection already contains...

so I think the code must be:

configuration.getMappedStatement(statementId, **false**)

and I wonder, when the incompleteStatements is build success, why not clear them?

The org.apache.ibatis.session.Configuration#buildAllStatements

/*
   * Parses all the unprocessed statement nodes in the cache. It is recommended
   * to call this method once all the mappers are added as it provides fail-fast
   * statement validation.
   */

It says that the method recommend to be called once, but in the code, there are many place call the org.apache.ibatis.session.Configuration#getMappedStatement(java.lang.String) method, it means if it parse the mapper xml and produce the incompleteStatements, when run the code it will throw already have mapped statement exception?

@lythesia

This comment has been minimized.

Copy link

lythesia commented Nov 8, 2018

+1

I cannot understand why it has to be invoked twice.
But the root reason is here:
This is the snip from parsePendingResultMaps (and it's same structure with parsePendingStatements

    synchronized (incompleteResultMaps) {
      Iterator<ResultMapResolver> iter = incompleteResultMaps.iterator();
      while (iter.hasNext()) {
        try {
          iter.next().resolve();
          iter.remove();
        } catch (IncompleteElementException e) {
          // ResultMap is still missing a resource...
        }
      }
    }

The pending-dealing procedure is invoked after current xml mapper is parsed, but its logic is WRONG!
Consider this xml mapper dependency graph A<--B<--C, apparently you should do parsePendingXXX in recursive fashion when parsing C, or you have a chance to get A left incomplete! And then aforementioned exception is raised when buildAllStatements hit twice.

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Nov 8, 2018

If you are having a problem, please provide a test case or example project so that we can reproduce the problem.
It could be caused by a wrong configuration like #1194 , so we need to investigate first.

@lythesia

This comment has been minimized.

Copy link

lythesia commented Nov 9, 2018

I cannot upload the full jar, but I can describe the case:

  1. You have all 3 mapper xml: A.xml, B.xml and C.xml
  2. A.xml has resultmap(say rm-a) defined, which extends rm-b defined in B.xml, and also rm-b extends rm-c defined in C.xml
  3. Now you need to load 3 xml and parse in this order: A, B, C
  4. After parsing done, once any mapper's method invoked, you will get the exception: java.lang.IllegalArgumentException: Result Maps collection already contains value for rm-a

harawata added a commit to harawata/mybatis-issues that referenced this issue Nov 9, 2018

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Nov 9, 2018

@lythesia ,

You might be surprised, but most reports without a repro do not reproduce the problem because there is a missing piece or two which looks irrelevant to the reporter.
That's why we ask reporters to provide an executable test case or example project.

Here is a project I created based on your report and it works without an error.
https://github.com/harawata/mybatis-issues/tree/master/gh-1176
Please modify it to reproduce the exception and upload it to your repo or send a pull request.

@lythesia

This comment has been minimized.

Copy link

lythesia commented Nov 10, 2018

@harawata Sorry for missing some details: I invoked the mapper's method via MapperMethod (as defined in mapper interface)

Here's the full test case, just a little modification based on yours.
mybatis-test.tar.gz

@lythesia

This comment has been minimized.

Copy link

lythesia commented Nov 10, 2018

@harawata PR sent for check.

@harawata harawata changed the title Configuration#buildAllStatements() call twice when there have incompleteStatements Deeply nested (3+ levels) result map could cause IllegalArgumentException Nov 17, 2018

@harawata harawata self-assigned this Nov 17, 2018

@harawata harawata added the bug label Nov 17, 2018

@harawata harawata added this to the 3.5.0 milestone Nov 17, 2018

@harawata harawata closed this in 3cacf14 Nov 17, 2018

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Nov 17, 2018

It should be fixed in the latest 3.5.0-SNAPSHOT.
If there still is a similar problem (it's possible), please create a test case (see the ones I created).

@lythesia ,
Thanks again for the repro!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment