[NEW RELEASE] MyBatis Plugin 1.0.0

@ganncamp Can you help to see this topic? Thank you!

Hi,

Welcome to the community.

You should know that, 2.5 days is not a long time to wait for a response on this type of request, which involves more than perhaps a little research and writing an answer.

I’ve requested some changes to your PR. Additionally, I’d like to talk about your rules. I see two groups of rules.

Group 1
Because you have no real rule descriptions, except repeats of the tiles themselves, you must remove all ambiguity from these titles.

  • “delete statement may not has where condition”
  • “select statement may not have where condition”
  • “update statement may not has where condition”

There are two ways to read these titles: “may not” could be either “is not allowed to” or it could be “it is possible that it does not”. I’m guessing you mean the second one, but in the context of rule titles “should not” and “may not” are generally interpreted as the first one. Thus it seems that your rules are saying “You must remove the where condition from this SQL statement”, which is not at all desirable.

If there is truly ambiguity in your rule implementation - i.e. a where condition could be present but your rule couldn’t find it - then I suggest wording like “Where condition not found in [statement type] statement”

If there is no ambiguity, and you know for certain that the issue is raised only when the where clause is absent, then I suggest ‘[statement type] statement must have a “where” condition’

As a best practice for rule writing, your rule descriptions would ideally contain a little bit of information about why where clauses are needed and what will go wrong if they’re absent. But expanding the descriptions is optional.

If you’ve held back on writing rule descriptions because English is not your native language, feel free to use a translator and ask here for feedback on the results. :slight_smile:

Group 2
My issue with these rules is not a blocker for Marketplace entry.

  • “delete statement should not include 1=1”
  • “select statement should not include 1=1”
  • “update statement should not include 1=1”

For this group, I understand the intent of the rule. However, I question the fact that you raise an issue when 1=1 is not the entire where clause. E.G. my test:

where id = #{id} and 1=1

I grant that this is useless code and a bad practice, but when it’s not the only condition in the where clause, it’s nearly harmless. Of course if you feel that this really is a Critical Bug in all circumstances, then a rule description would be the place to explain why. :wink:

Documentation
I would also like to see you update your documentation slightly. You talk very prominently about <if test=""> not being matched (matched by what? an else?) but as far as I can tell, your rules test only the raw SQL, not not this logic. (Or am I wrong about that?) So you should make clear in your documentation that this unmatched logic is not tested by your rules. Either that or remove mention of it.

 
Ann

I’m glad to receive your reply.
Thanks for your suggestion, I have updated the code and document.

For the rules of Group 1, I apply for suggestion: “Where condition not found in [statement type] statement”.
Also I add the description, for example, descrption of rule Where condition not found in delete statement as follows:
If all parameters in the delete statement of Mapper XML file are null, the sql will not have where condition, then it will delete all records from the table, which will result production accident. Suggest using native dynamic sql instead of MyBatis dynamic SQL for required parameters.

For the rules of Group 2, As you said, it is a useless code and bad practice, so suggest removing it. Also I have updated the description.

For the Document, I also updated it, so that make it much easier to understand.

Thanks your suggestion again.

Ann

Hi,

I need one more change in your PR (commented in GH), and I’m not seeing the rule changes you describe. Could you put out a point release with them and update your PR to point to that version instead of this one?

 
Thx,
Ann

Hi,

I have update the PR and new version 1.0.1 has been released.
Then it still need your review.

Thank you!

Hi,

The new release looks good! Thanks for making the changes. :slight_smile:

We just need one more change to the PR.

 
Ann

Hi,

I’m very glad to hear this news now.
And Thanks you very much!

Ann

You’re in!

 
:slight_smile:
Ann