Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support for customized ProviderSqlSource. #1229

Closed
wants to merge 1 commit into from

Conversation

abel533
Copy link
Contributor

@abel533 abel533 commented Mar 25, 2018

Enhances the functionality by customizing the ProviderSqlSource.

Abstract classes used to defined Construction methods.

The two PR(the other is #1226) are not very different, do you have any idea or better implementation?

@h3adache @kazuki43zoo @chb0github

@chb0github
Copy link

None of my PRs have been accepted. No one even seems to respond to my requests. There is currently a full implementation pending from one of my colleagues that supports mysql

@chb0github
Copy link

His name is pj

@jeffgbutler
Copy link
Member

Why are there two nearly identical PRs? Maybe just pick one and close the other one?

@abel533
Copy link
Contributor Author

abel533 commented Mar 25, 2018

@jeffgbutler The two PRs differ in AbstractProviderSqlSource. Which one is better?

@jeffgbutler
Copy link
Member

I'm having a hard time understanding the problem you are trying to solve here. The current implementation allows you to call any Java method. That already gives you tremendous flexibility for generating SQL.

The test you submitted shows generating a script that uses the MyBatis XML tags - which will in turn be executed by MyBatis to generate SQL. Why wouldn't you just generate SQL directly?

Is there any other use case for this other than generating scripts that use the MyBatis XML tags?

@abel533
Copy link
Contributor Author

abel533 commented Mar 25, 2018

In order to use XML tags to handle dynamic SQL. In this way you can simply return a DynamicSqlSource without having to deal with too much Java logic.

@jeffgbutler
Copy link
Member

OK - you've confirmed what I was thinking. It still seems weird to me in that you are generating a script that will generate SQL - a meta generator if you will :)

I think a better approach would be to expose the XML scripting engine so that it could be called directly. Then you could use the existing SQL provider mechanism.

@abel533
Copy link
Contributor Author

abel533 commented Mar 25, 2018

Can not get configuration in provider implementation method, if provided, can directly call languageDriver.createSqlSource(configuration, originalSql, parameterType);

@jeffgbutler
Copy link
Member

I see that. I suppose you would need to do something like this to be able to reuse the XML language driver.

If I recall correctly, this is the 4th pull request you've submitted for this function, and there are currently two open PRs. I suggest that you pick the one implementation you think is best and close the other. Then we can all review it and make suggestions in one place.

My vote is +0. I would be interested in what the other committers think.

@abel533
Copy link
Contributor Author

abel533 commented Mar 26, 2018

The abstract class(AbstractProviderSqlSource) of this PR only limited the constructor. Another PR abstract class provides the basic logic. I think the other(#1226) may be more better. close this PR(#1229)?

@jeffgbutler
Copy link
Member

Sounds good - close this one, then we'll review #1226.

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

Successfully merging this pull request may close these issues.

3 participants