Remove mapper order restriction when referencing SQL fragment in another file.

[GoogleCodeExporter]

What version of the MyBatis are you using?
3.0.3

Please describe the problem.  Unit tests are best!
Example mappers:

- PersonMapper.xml
<sql id="columns">person_id, person_name</sql>
<select id="selectPersonWithPets">
SELECT
<inculde refid="columns" />
<include refid="mapper.PetMapper.columns" />
FROM Person LEFT JOIN Pet ON ....
</select>

- PetMapper.xml
<sql id="columns">pet_id, pet_name, pet_owner_id</sql>
<select id="selectPetWithOwner">
SELECT
<inculde refid="columns" />
<include refid="mapper.PersonMapper.columns" />
FROM Person, Pet WHERE ...
</select>

What is the expected output? What do you see instead?
- Expected:
Both sql fragments are resolved correctly.

- Actual:
IllegalArgumentException (XML fragments parsed from previous mappers
does not contain value for...)

Please provide any additional information below.
This is beneficial to mapper auto-detection mechanisms.

Original issue reported on code.google.com by haraw...@gmail.com on 22 Nov 2010 at 3:24

[GoogleCodeExporter]

Attached a patch against revision 3280.

[Changes]

- When adding a mapper, all the statement nodes (i.e. 
select,update,insert,delete) are stored into a temporary map without being 
processed.

- There are basically two ways to parse the cached statement nodes:
 (1) By accessing Configuration.mappedStatement (e.g. via getMappedStatement(String)). In this case, only the affected nodes are processed (per namespace basis).
 (2) By calling Configuration#buildAllStatements(). With this method, all the cached statement nodes are processed. It is recommended to call this method once all the mappers are added as it provides fail-fast statement validation.

- To pass existing tests, I had to change a line in a sqlmap 
(src/test/java/org/apache/ibatis/submitted/parent_child_circular/NodeMapper.xml)
 as its 'namespace' value was mismatched. The patch includes this change.

[TODOs]
- Test case for cache related configuration.
- See if there are any existing code where calling buildAllStatements() is 
preferable.

Original comment by haraw...@gmail.com on 23 Nov 2010 at 2:10

Attachments:

• issue179.patch

[GoogleCodeExporter]

One more TODO:
- Re-evaluate the necessity of  'resourceForNamespace'.

Original comment by haraw...@gmail.com on 23 Nov 2010 at 3:22

[GoogleCodeExporter]

Please use this patch (I will remove the first one if I can).

Changes from the first version:
- Made 'cache-ref' order independent.
- Added more tests.
- Other improvements.

The notes in comment 1 is still applicable (except TODOs).

Original comment by haraw...@gmail.com on 24 Nov 2010 at 3:41

Attachments:

• issue179_2.patch

[GoogleCodeExporter]

Original comment by christia...@ircm.qc.ca on 26 Nov 2010 at 12:47

• Changed state: Started
• Added labels: Version-Release3.x, Component-SqlMaps

[GoogleCodeExporter]

Great patch!
I've added a couple of complex tests and everything was OK!

Original comment by christia...@ircm.qc.ca on 26 Nov 2010 at 2:36

• Changed state: Fixed

[GoogleCodeExporter]

Thanks a lot for adding so many tests and committing the patch!
I have been wanting to remove the order dependency since version 2.x, actually 
;-)

# I noticed that the mapper order still matters when extending a resultMap in 
another file, but this shouldn't be a big problem because we usually define the 
child resultMap in the same file.

Original comment by haraw...@gmail.com on 26 Nov 2010 at 6:08

[GoogleCodeExporter]

Even with this problem, I believe the situation is better than it was.
We may correct this in the future, especially if someone adds it as a bug.

Original comment by christia...@ircm.qc.ca on 26 Nov 2010 at 7:50

[GoogleCodeExporter]

I totally agree.

Original comment by haraw...@gmail.com on 27 Nov 2010 at 5:30

[GoogleCodeExporter]

Christian,

I assumed the namespace of a mapper is always fully qualified, but it actually 
can be any string.
As a result, there is a small issue in the current trunk; Although statements 
are executed correctly, wrong resource name is output to the log when an error 
occured during a query.

I have attached a test case and a patch for the issue.
Could you review them when you have a time?

Thanks,
Iwao

Original comment by haraw...@gmail.com on 4 Dec 2010 at 6:56

Attachments:

• NonFQNamespaceTest.zip
• issue49_namespace_fix.patch

[GoogleCodeExporter]

And there must be a problem when referring a statement without namespace (short 
name resolution).
I'll look into it tomorrow...

Original comment by haraw...@gmail.com on 4 Dec 2010 at 7:33

[GoogleCodeExporter]

The attached patch includes the following changes and test cases.

- Fixed: incorrect resource name is output to the log when the mapper has a 
non-fully-qualified namespace.
- Fixed: a short name of a statement was not resolved correctly.

Original comment by haraw...@gmail.com on 5 Dec 2010 at 4:12

Attachments:

• issue49_addition.patch

[GoogleCodeExporter]

Thanks for the patch!
I've seen trouble with namespace in 3.0.3 release. I use <sql id="columns"> in 
many different mappers and they collided with an awful error message. Maybe the 
changes I made with issue 133 didn't turn out well.
Fortunately, the problem had already been solved in the trunk version.

I'll have a look at your patch this week.

Original comment by christia...@ircm.qc.ca on 6 Dec 2010 at 2:04

• Changed state: Accepted
• Added labels: Usability

[GoogleCodeExporter]

That patch gave me a headache... I really don't think anyone should use a space 
inside a namespace. But if it works, then good for us!
Thanks again for the patch, very appreciated!

Original comment by christia...@ircm.qc.ca on 10 Dec 2010 at 1:45

• Changed state: Fixed

[GoogleCodeExporter]

Thank you Christian!
I tend to choose the craziest scenario for a test case, sorry about that ;)
And for the record, I personally recommend using fully-qualified namespace.

Original comment by haraw...@gmail.com on 11 Dec 2010 at 10:13

[GoogleCodeExporter]

Original comment by eduardo.macarron on 30 Dec 2010 at 8:43

• Added labels: Target-Release3.0.4

[GoogleCodeExporter]

Reopened issue since there is a problem when running the first statement from 
multiple threads.

Original comment by christia...@ircm.qc.ca on 6 Jan 2011 at 7:32

• Changed state: Started

[GoogleCodeExporter]

Issue 221 has been merged into this issue.

Original comment by eduardo.macarron on 6 Jan 2011 at 9:21

[GoogleCodeExporter]

Merged issue 221. The problem is related to this one.

Original comment by eduardo.macarron on 6 Jan 2011 at 9:22

[GoogleCodeExporter]

Christian,

Thanks for investigation.
Have you or Eduardo already started working on the fix?
I will look into it if you haven't.
It would be helpful if you have a test case or steps to reproduce.

Original comment by haraw...@gmail.com on 7 Jan 2011 at 2:19

[GoogleCodeExporter]

Hi harawata. Yes, I think Christian is hands on it. There has been some 
discussion on the dev list that would be interesting in case you want to 
contribute.

A user sometimes gets this exception on the first hit: 
java.lang.IllegalArgumentException: Mapped Statements collection already 
contains value for 
com.pikeelectronic.clan.server.ClanMapper.selectContextGroupById

http://groups.google.com/group/mybatis-user/browse_thread/thread/cac25e71f38295d
b

It seems that there is a synchronization problem in Configuration because 
synchronizing getMappedStatement the problem was gone. Unfortunately it is not 
easy to reproduce with a test case.

  public MappedStatement getMappedStatement(String id) {
    if (!mappedStatements.containsKey(id)) {
      buildStatementsFromId(id);
    }
    return mappedStatements.get(id);
  }

There are other methods in Configuration that access mappedStatements map that 
should be synchronized also.

We thought that given that these methods are used very often, that 
synchronization may have a performance penalty. So we wonder if all this 
initialization work could be done during startup when there are no concurrency 
problems.

Original comment by eduardo.macarron on 7 Jan 2011 at 7:23

[GoogleCodeExporter]

Eduardo, Thanks a lot for the detailed explanation.
ConcurrentModificationException may make sense, but IllegalArgumentException 
looks strange...
I'll ask the reporter the sample app and see what's going on.

# Off the top of my head, what happens if the StrictMap class extends 
ConcurrentHashMap instead of HashMap?

And thank you for your hard work on mybatis!

Regards,
Iwao

Original comment by haraw...@gmail.com on 7 Jan 2011 at 8:11

[GoogleCodeExporter]

Hi again Iwao.

The concurrency problem is at a higher level. 

After the startup has finished worker threads start their work. In the case of 
this user it has few statements so it is easy that both threads call 
getMappedStatement at the same time.

Both them get false on mappedStatements.containsKey(id) but just the one that 
first reaches the mappedStatements.add(id) works, the other fails because 
StrictMap does not let it to overwrite the property.

Original comment by eduardo.macarron on 7 Jan 2011 at 12:14

• Added labels: Target-Release3.0.5
• Removed labels: Target-Release3.0.4

[GoogleCodeExporter]

I've worked on a patch. You can have a look at it. It's really in a primitive 
state and fails, but it's a starting point.

The first file contains a new class which is an aggregate of 
XMLStatementBuilder and XNode that contains the statement.
Tests run: 1285, Failures: 16, Errors: 268, Skipped: 3

The second patch integrates the XNode into XMLStatementBuilder.
Tests run: 1330, Failures: 2, Errors: 0, Skipped: 8

I haven't looked at why there are errors, but I wished to shared my code so you 
can have a look at it.

Original comment by christia...@ircm.qc.ca on 7 Jan 2011 at 2:44

Attachments:

• mybatis_179_aggregate.txt
• mybatis_179_noaggregate.txt

[GoogleCodeExporter]

I've corrected all bugs I could find. Still, some tests are skipped on my 
computer. Eduardo, are you able to run all test cases?

Original comment by christia...@ircm.qc.ca on 7 Jan 2011 at 4:58

Attachments:

• mybatis_179_noaggregate_wcache.txt

[GoogleCodeExporter]

This should be close to the final patch to use.

Original comment by christia...@ircm.qc.ca on 7 Jan 2011 at 8:59

Attachments:

• mybatis_179_noaggregate_wcache_4.txt

[GoogleCodeExporter]

Christian. I have added the patch to fix the spring module problem with this 
code (over your patch)

The test I have added failed with 3.0.3, worked with 3.0.4 and fails after 
applying your patch.

Basically the change is that resources map just holds namespaces loaded from 
resources (not from mappers), instead of resource names. Resource names are 
just used for error reporting.

Original comment by eduardo.macarron on 7 Jan 2011 at 10:18

Attachments:

• issue_179_plus_spring_fix.patch

[GoogleCodeExporter]

Sorry for a late reply and thank you both for your time.

It turned out to be a busy weekend and I haven't done anything useful.
I have just confirmed that no tests are failed with the patch that is attached 
to comment 26.
If there is anything I can do to help, please let me know.

Regards,
Iwao

Original comment by haraw...@gmail.com on 9 Jan 2011 at 5:49

[GoogleCodeExporter]

I noticed the same problem, and now this issue is resolved it?

Original comment by GeorgeNi...@gmail.com on 10 Jan 2011 at 9:53

[GoogleCodeExporter]

The goal of the last patch is to correct the problem of multiple thread calling 
Configuration.getMappedStatement() at the same time. The first time 
Configuration.getMappedStatement() is called, all statements are actually 
parsed which result in errors for any thread calling the method while 
statements are being parsed.

Still, this patch is not the final one. I still have to change a couple of 
things like replacing HashMaps for StrictMaps and improving the error messages 
that appear. Another thing that may need improvement is that I am not convinced 
that resultMaps are handled correctly. For example, a statement/resultMap 
uses/extends a resultMap from an unloaded mapper.

Original comment by christia...@ircm.qc.ca on 10 Jan 2011 at 2:27

[GoogleCodeExporter]

Bug corrected in revision 3586.

Original comment by christia...@ircm.qc.ca on 24 Jan 2011 at 7:43

• Changed state: Fixed

[GoogleCodeExporter]

Thank you Christian for the fix.

It would be obvious, but mybatis-guice implementation may need some adjustment 
as the Configuration#buildAllStatement() is changed to 'protected'.

Regards,
Iwao

Original comment by haraw...@gmail.com on 25 Jan 2011 at 8:40

[GoogleCodeExporter]

Thanks for the side note. I didn't look at the mybatis-guice recently so I 
overlooked that.
There were much debate in the dev mailing list as to how the code should be 
changed to keep order optional but to prevent users from having to call a 
"validate" method. So that is the reason buildAllStatement() became protected.

Original comment by christia...@ircm.qc.ca on 25 Jan 2011 at 1:57

[GoogleCodeExporter]

When using a resultMap from an unloaded mapper, an exception is raised since 
the resultMap is resolved immediatly.
I've fixed the issue in revision 3623.

Original comment by christia...@ircm.qc.ca on 14 Feb 2011 at 5:05

[GoogleCodeExporter]

Issue 266 has been merged into this issue.

Original comment by eduardo.macarron on 13 Mar 2011 at 3:29

[GoogleCodeExporter]

Issue 291 has been merged into this issue.

Original comment by eduardo.macarron on 2 Apr 2011 at 4:33

[GoogleCodeExporter]

Christian, have a look at issue 291.

XMLStatementBuilder$SelectKeyHandler#handleNode has a call to 
configuration.getMappedStatement(). If there is any uncomplete statement loaded 
this method will always fail.

Maybe that call to builAllStatements inside getMappedStatement should be 
removed?

Original comment by eduardo.macarron on 2 Apr 2011 at 4:39

• Changed state: Accepted

[GoogleCodeExporter]

I don't think it should be removed since it some code in MyBatis uses 
configuration.getMappedStatement() and should fail is an incomplete statement 
is present.

What I expect to do is to add a configuration.getMappedStatement(String, 
Boolean) method so that validation can be skipped while building the 
Configuration object.

Original comment by christia...@ircm.qc.ca on 4 Apr 2011 at 4:11

[GoogleCodeExporter]

Solved in revision 3726.

Original comment by christia...@ircm.qc.ca on 4 Apr 2011 at 5:09

• Changed state: Fixed

[GoogleCodeExporter]

Issue 305 has been merged into this issue.

Original comment by eduardo.macarron on 23 Apr 2011 at 8:13

[GoogleCodeExporter]

Issue 492 has been merged into this issue.

Original comment by eduardo.macarron on 9 Jan 2012 at 2:37