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

XSD support in mapper/config XML #1393

Merged
merged 6 commits into from
Dec 5, 2018

Conversation

harawata
Copy link
Member

@harawata harawata commented Dec 4, 2018

A fix for #1193 .
Mostly @kazuki43zoo 's work. :D

By setting system property org.mybatis.useXsd to true, users can use XSD instead of DTD when writing mapper/config XML.

<mapper namespace="org.apache.ibatis.builder.xsd.AuthorMapper"
  xmlns="http://mybatis.org/schema/mybatis-mapper"
  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
  xsi:schemaLocation="http://mybatis.org/schema/mybatis-mapper http://mybatis.org/schema/mybatis-mapper.xsd">
<configuration xmlns="http://mybatis.org/schema/mybatis-config"
  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
  xsi:schemaLocation="http://mybatis.org/schema/mybatis-config http://mybatis.org/schema/mybatis-config.xsd">

Migration steps would be....(just an idea)

  1. In version 3.5.0, keep DTD as the default.
  2. In version X (when some XSD-only enhancement is added?), change the default to XSD and output a log message (INFO or WARN) like DTD is deprecated, to use XSD, see ... to encourage users to migrate to XSD..
  3. In version Y, remove DTD support.

@jeffgbutler ,
This may affect Generator in some way.
Please let me know if there is any concern.

kazuki43zoo and others added 6 commits March 14, 2017 10:42
# Conflicts:
#	src/main/java/org/apache/ibatis/builder/xml/XMLMapperBuilder.java
#	src/main/java/org/apache/ibatis/parsing/XPathParser.java
# Conflicts:
#	src/main/java/org/apache/ibatis/builder/xml/XMLMapperEntityResolver.java
@jeffgbutler
Copy link
Member

I think there would only be a generator impact in your "version Y" when DTD support is removed. Also, the newer code generated for my MyBatis Dynamic SQL library doesn't use XML at all, so no issue there. I'm OK with this change.

@harawata
Copy link
Member Author

harawata commented Dec 5, 2018

Thanks for the comment, @jeffgbutler !
Let's test this in the snapshot, then.

@harawata harawata merged commit 1a988cf into mybatis:master Dec 5, 2018
@harawata harawata added the enhancement Improve a feature or add a new feature label Dec 5, 2018
@harawata harawata added this to the 3.5.0 milestone Dec 5, 2018
harawata added a commit to mybatis/mybatis.github.io that referenced this pull request Dec 5, 2018
@jeffgbutler
Copy link
Member

I haven't reviewed this code (sorry) - but I have a question...is it really necessary to have the system property? Can the code be written such that MyBatis will parse the file if either XSD or DTD is used?

@harawata
Copy link
Member Author

harawata commented Dec 5, 2018

@jeffgbutler ,
I couldn't find a way to achieve it.
Any approach we should try?

@jeffgbutler
Copy link
Member

I don't have much of an idea. I suppose you could try DTD and if it failed, fall back to XSD. But that seems kind of kludgy.

@harawata
Copy link
Member Author

harawata commented Dec 6, 2018

I tried that approach, but it wasn't possible to re-read the Reader/InputStream unless we put the entire file content on memory.

I then tried to find a lenient way to parse both XML using the same code, but couldn't find any.
It is possible that I have missed it as there are many different way to parse XML, though.

@harawata
Copy link
Member Author

harawata commented Jan 7, 2019

@jeffgbutler ,
We're approaching the release of 3.5.0.
In case you are still unsure about this, I am totally fine with reverting the change and pushing the XSD support to a future milestone.
If there is a way to remove the system property, I would be more than happy. :)

@jeffgbutler
Copy link
Member

I haven't had time to look at it, so don't hold up the release for me.

My only idea is as I said above - try it one way and if it fails try it the other way. This would mean reading the file twice, so then I could imagine a property like "preferXsd" which isn't any better.

I will say that Spring supports both ways without a property - so it must be possible. Not sure how they do it.

@harawata
Copy link
Member Author

harawata commented Jan 8, 2019

Thanks for the comment, @jeffgbutler !

The problem is that it is InputStream that is passed around in the current implementation.
And there is no good way to read the same InputStream twice without storing its content on memory.

Spring seems to use org.springframework.core.io.Resource as a parameter to avoid this problem.

https://github.com/spring-projects/spring-framework/blob/109552d86853bbf4b026bd556842f7ffa78d281b/spring-beans/src/main/java/org/springframework/beans/factory/xml/XmlBeanDefinitionReader.java#L461-L495

It would be possible to change MyBatis implementation, but the impact won't be small.
Rather than delaying 3.5.0 release, I would revert this PR and revisit the new approach later.
This change by itself provides no advantage anyway.

@kazuki43zoo ,
What do you think?

@jeffgbutler
Copy link
Member

@harawata +1 on revert/delay. I'd rather we solve this in a more Spring-like way, but don't need to delay the release for that.

@kazuki43zoo
Copy link
Member

@harawata @jeffgbutler I agree!

harawata added a commit that referenced this pull request Jan 9, 2019
We try to find a solution that does not require system property switch. We may be able to reuse the XSD files and the most part of the tests.
@harawata harawata removed this from the 3.5.0 milestone Jan 9, 2019
@harawata
Copy link
Member Author

harawata commented Jan 9, 2019

Reverted. Thank you for the comments!

@kazuki43zoo kazuki43zoo added invalid and removed enhancement Improve a feature or add a new feature labels Apr 14, 2019
@kazuki43zoo kazuki43zoo removed their assignment Apr 14, 2019
pulllock pushed a commit to pulllock/mybatis-3 that referenced this pull request Oct 19, 2023
pulllock pushed a commit to pulllock/mybatis-3 that referenced this pull request Oct 19, 2023
We try to find a solution that does not require system property switch. We may be able to reuse the XSD files and the most part of the tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants