Found a real hammer bug in Spring affairs, and the official refused to admit it? You come to judge...

Hello, I am crooked.

Here's the thing. Last week, I was concentrating on fishing, and then a small friend sent me a WeChat message, asking a question about the affairs, and adding two pieces of code:

Conclusion first: I think this is a bug with Spring transactions. But the official said that this can only be regarded as a defect in the documentation, not a bug in the code.

(Well, I wrote this article for several days, so when I wrote the above sentence, the official did not admit that it was a bug, but after writing it, they also admitted that it was indeed a code defect. It does not affect, then look down .)

Good guy, I understand, all interpretation rights belong to the official.

Before I get to the bottom of it, I want to say a few words about how to ask this question.

I cut out the reader's question above because I think this question is simply a template-like question.

Give a piece of sample code, give a piece of source code, explain your problem, and indicate that you have queried but have not found a suitable answer.

After I read his text, I can quickly get what his problem is, so the communication between us is very efficient.

In the end, we did not discuss a reasonable explanation, so he went to raise an issue, hoping to get an official and more authoritative answer.

So let's start our story around this issue.

Stage construction

Before the main show begins, I will first set up the stage for you, that is, the demo.

Because it is about Spring transactions, so this Demo mainly reflects the application of "transactions".

So the core thing in the demo is this part:

.png)

The two exceptions involved are simple custom exceptions:

Let's say there's a weird site that only allows users between the ages of 10-18. This part represents the user registration feature of the site.

Then we insert a piece of data into the user_info table, and then judge that if the age is greater than 18 years old, an AgeExceptionOver18 exception will be thrown, indicating that the user is not the target user.

But you pay attention, the rollbackFor in my @Transactional annotation is AgeException.class, which means that I don't want to roll back the user who is older than 18. I still want to save his registration information, just throw an exception to indicate that he is not my target user,

When we insert a user younger than 10 years old, AgeException.class will be thrown, and the insert statement just executed should be rolled back. I don't want to save this part of the user's information.

Ok, now some friends will ask: Since users younger than 10 years old don't want to save, why not judge before inserting?

Very good question, in actual development, it must be judged before inserting, but I am just here to demonstrate the transaction function, so I wrote it like this, what the hell!

For the above code, let me make an interface to trigger it, that is, make a Controller:

The above four categories are the most critical ones, so I will take them out separately.

The entire project structure is also very simple:

The other classes are not critical, so I won't mention them. They are all your best cruds. It's not too much to spend five minutes on this project, right? You can also touch the fish for two minutes in the middle.

I adjusted the log level to debug level, and then ran the project to verify the functionality.

Then call this link:

http://127.0.0.1:8085/insertUser?age=8

The corresponding log is as follows:

You can see the part I framed, first confirm that the insert statement is executed, and the age is indeed 8. But finally Rolling back, that is, rolled back.

Why do we roll back, we are also clear in our hearts, because it echoes here:

Next try age for 18-year-old users:

http://127.0.0.1:8085/insertUser?age=18

The corresponding log is as follows:

This is nothing to say, the transaction is successfully committed, and the data is inserted successfully. is our expected result.

There is nothing wrong with the database data:

Then try age for a 28 year old user.

Our expectation for this user is to throw an AgeExceptionOver18 exception, but the data has to be inserted successfully.

Come and go one:

http://127.0.0.1:8085/insertUser?age=28

The corresponding log is as follows:

First, the data was actually rolled back? ? ?

The exception was thrown, but it didn't echo!

No matter what the principle is, from my point of view, first of all, my @Transactional annotation usage is absolutely correct, the transaction configuration is absolutely correct, and my exceptions are not thrown around. Why did you roll it back for me?

Are you saying it's not a bug?

Just change the documentation?

The implication is to "deny" and forcibly find a remedy from the document?

Isn't this bullying honest people?

Um... wait, that's how it was when I wrote this.

But after I finished writing this paragraph, I went to the issues again the next day and found that things had changed. The official admitted that this was a bug. The description on the document will be revised in version 5.3.x, and it will be updated in version 6.0. There are code fixes in it.

But I have prepared so much in front of me, and I have already written it, so I will not change it, just leave a trace of my creation here, haha.

Let's look down.

dramatic conflict

A play must have its dramatic conflict, which is the core part of it. So what is the core conflict in our demo?

This subsection will first tell you where the "dramatic conflict" is.

Let me ask you a question first:

For Spring-managed transactions, what is the default rollback exception?

We took this question to see the source code, found the answer to this question, and you can slip into the play.

First make a breakpoint, run the program, and then look at the call stack:

You can see that there is such a method related to the transaction in the call stack:

org.springframework.transaction.interceptor.TransactionAspectSupport#invokeWithinTransaction

This is our breakthrough.

What, you ask me how I found this place all at once?

All I can say is: practice makes perfect.

Well, there are actually skills, you can try to find them yourself, because this is not the focus of this article, so I won't say more.

After the method is executed abnormally, it will go to the catch code block. The following line of code is the entry for the exception-related processing:

In our age=28 scenario, after this method comes in, the first ex parameter is our custom AgeExceptionOver18 exception:

I also framed a line of code:

txInfo.transactionAttribute.rollbackOn(ex)

You can see the name rollbackOn in this line of code, and you know that it is to judge whether the ex parameter matches the specified rollback exception.

What if it matches? What if it doesn't match?

If it matches, rollback.

Commit if it doesn't match.

Alright, let's look down.

You will come here:

org.springframework.transaction.interceptor.RuleBasedTransactionAttribute#rollbackOn

In this method, when the winner is null, there is a comment on it, which means that the corresponding rule is not matched.

That is, we don't configure anything, and by default, winner is null.

Then the following line of code hides the answer to the question we are looking for:

return super.rollbackOn(ex);

So what is the default rollback exception for Spring-managed transactions?

The source code tells me that if the currently thrown exception belongs to RuntimeException or Error, it will be rolled back.

I was paving the way up front, just to lead you to the rollbackOn method. Even the super.rollbackOn(ex) line of code is a smoke bomb, and we don't need to pay attention to it in this article.

What we need to focus on is this part:

First of all, let's clarify what this rollbackRules is.

In our demo, it is the full path name of the AgeException.class object we configured in the rollbackFor attribute of the @Transactional annotation:

Well, the next thing is the key, you must cheer up.

Focus on this line of code:

int depth = rule.getDepth(ex);

Come, let's see what rule and ex are respectively:

The exceptionName in rule is the full path name of the AgeException object we configured.

ex is the AgeExceptionOver18 exception thrown by our program.

The core conflict of this scene is hidden in this method here:

org.springframework.transaction.interceptor.RollbackRuleAttribute#getDepth(java.lang.Throwable)

Well, so far, the stage is set up, and the core conflict has been exposed.

The show is ready to begin.

The curtain opens

Look carefully at the code I've framed.

What is the previous exceptionClass.getName()?

It looks like this:

What is this.exceptionName behind?

It's this thing:

Next, something magical is about to happen, Tiezi.

com.example.transactional.exception.AgeExceptionOver18
com.example.transactional.exception.AgeException

Although these are two different exceptions, these two strings carry out the contains operation. Do you mean to return true?

So, depth here will return 0.

Then the winner here will not be empty

So the return value of this method will be true:

Remember what I said earlier, what code will be executed when returning true here?

Is it just rollback?

Therefore, the source of all evil is this piece of code that we mentioned when the curtain opened:

org.springframework.transaction.interceptor.RollbackRuleAttribute#getDepth(java.lang.Class<?>, int)

At this point, I think it's pretty clear: isn't this a bug? You are better than Spring Do you still want to quibble?

However, if the following two strings are equals, do you say that it returns false, and the problem is solved?

com.example.transactional.exception.AgeExceptionOver18
com.example.transactional.exception.AgeException

The reason is such a truth, but I think the problem is definitely not so simple.

First of all, I think the use of contains here must be intentional, but for what purpose, I'm really not sure.

So the readers I discussed with me put forward a view, will it be to satisfy the property of rollbackForClassName:

Because when we use rollbackForClassName, we can configure multiple exception names that need to be rolled back in the form of a string array. For example, I make a NullPointerException:

In normal use scenarios, we can complete the rollback operation.

The value of the code in the corresponding place is as follows;

The java.lang.NullPointerException string of course contains the NullPointerException string. So let's roll back. No problem.

But if we use the equals operation, it will not match, causing the rollbackForClassName property to fail.

Therefore, changing the contains to equals is a measure to demolish the west wall and repair the east wall, which is not advisable.

But the rollbackForClassName property has no effect in our Demo.

For example, if I change the program to this, you say, is it messed up?

Same thing.

The com.example.transactional.exception.AgeExceptionOver18 string of course contains the AgeException string.

But I don't want to roll back, brother, take a good look at it, the exception I threw is AgeExceptionOver18.

At this point, I think I should have described the problem very clearly. If you still don't understand what the problem is, then you don't need to look down, and then look at the section "The Curtain Opens".

Otherwise, it will be difficult for you to get into the play later.

pave the way

In order to better throw out the real problem, I must first introduce another related problem as a foreshadowing.

First, let's go to the issues of the Spring project and search for the RollbackRuleAttribute class where the getDepth method is located.

See if there are any relevant clues, and the results are as follows:

After analysis, only the first content was helpful to me.

https://github.com/spring-projects/spring-framework/pull/24682

The topic is called:

Improve javadoc in RollbackRuleAttribute regarding nested classes.
Improve the javadoc about nested classes in RollbackRuleAttribute.

From the title we know that this is an improvement to the documentation.

So what exactly is the improvement?

It can be seen that his description also mentioned the method of "the source of all evil" that we analyzed earlier.

Regarding what he said specifically, in fact, I don't need to translate it for you, just show you the code he submitted and it's clear at a glance:

He mainly talked about the problem of inner classes, and his problem is a little different from ours.

His two exception classes, one is called EnclosingException and the other is called EnclosedException, and there is no contains relationship between these two strings.

So in the context of inner classes, what's the problem?

I will also show you one, you only need to look at it to understand, the sample code is as follows:

It should be noted that my current two exceptions are AgeException and AgeOver18Exception, and there is no containment relationship between the two.

The previous demo was AgeExceptionOver18.

AgeOver18Exception
AgeExceptionOver18

Look no further.

You see, the exception is thrown when the inner class is like this:

throw new AgeException.AgeOver18Exception();

If you haven't recalled it, it doesn't matter, as soon as the breakpoint hits, the code runs and suddenly realizes:

Do you understand, Tiezi. The full path name of the exception thrown by the inner class is this:

xxx.UserInfoService\$ AgeException$AgeOver18Exception

Doesn't this include AgeException, doesn't it match, doesn't it roll back?

So, while his triggering of this question is not the same as what I mentioned earlier, the "root of all evil" is the same.

So what is the solution?

Just modified the documentation, and from the documentation point of view, this situation will be rolled back:

Corresponding to the source code, that is, the comments in this place:

org.springframework.transaction.interceptor.RollbackRuleAttribute#RollbackRuleAttribute(java.lang.Class<?>)

Ok, let's think about it calmly now, here is just to fix this problem from the perspective of the documentation. It is clearly stated in the documentation that the inner class with the specified exception will also be rolled back. Is this right?

I think reluctance is acceptable.

For example, we know that an exception class is marked as should be rolled back, then the subclass of this exception class should be rolled back, which is no problem.

I think inner classes and subclasses should keep the same logic. After all, they did have a code relationship before, and from this point of view, it was said in the past.

After all, all interpretation rights belong to the government.

Here you have to remember:

• The RollbackRuleAttribute class has already exposed the problem of inner classes because of the use of contains in the judgment of rollback exceptions.
• The problem was fixed by modifying the javadoc description, without modifying any code.
• This solution barely speaks for itself.

Alright, the paving is done.

good show

I searched for RollbackRuleAttribute in issues again and found one more content:

In fact, I deliberately concealed this content before, because this issue was raised by the readers I chatted with.

The sample code here is the code that appears at the very beginning of the article.

The good show is hidden in this issues, let's see how the official "repeatedly jumps".

https://github.com/spring-projects/spring-framework/issues/28098

First, a buddy named snicol changed the title of this issue:

The "Bug" at the beginning has been removed, which is also normal. It is a non-standard question. At this stage, the questioner only thinks it is a bug. If you choose a name like this, your personal subjective consciousness is too strong, which is not good.

Mainly, do you know who is the snicoll who revised the title?

Don't ask, ask is the big guy, the core maintainer of Spring and Spring Boot projects.

Then after a few days, the title of this question was simply modified by another big guy:

Just change use contains to uses contains() , and change equals to equals() .

This small detail perfectly reflects the rigor of the Spring framework, which can be said to be very rigorous.

I haven't entered the question answering link yet, so I first corrected the "typos" in the question.

Then we entered the official Q&A session.

I have said a lot of content below, but don't panic at all, as you know my English level is very high. This pile of content is divided into three parts, I will explain it to you little by little:

First the first part:

It's a long sentence in English at first, but don't be afraid at all.

You can see that he first mentioned a phrase "by design" succinctly, which is "by design".

The whole translation is probably like this.

Where should we use the contains() method? This has actually been considered.

So what is the consideration based on?

In XML configuration files, users typically specify the simple name of the custom exception type, rather than the full path class name.

What do you mean?

He gave an example of the xml configuration in the documentation:

Then I will also make an xml configuration based on our Demo, and go back to the xml-based configuration a few years ago:

Here I also have to sigh with emotion: it was really troublesome to develop based on xml in the past, and every time I had to go to the system project to copy a configuration, so I am still very grateful for the emergence of SpringBoot.

What does he mean here?

In my xml configuration, about the rollback-for attribute. The simple name he mentioned was AgeException. And the fully qualified class name is com.example.transactional.exception.AgeException.

That is to say, there is no restriction on what users can fill in.

If the user fills in the simple name, we should also make it take effect, so the contains() method must be used.

As far as I understand, this place is the same as the usage of the rollbackForClassName attribute in the @Transactional annotation, and this is a legacy issue and a bad design back then.

But I don't think it can be said to be ill-considered, after all, it is difficult for others to think that you would name exception classes in such a strange way!

In short, in this paragraph, he explained why the contains() method was used and why the equals() method could not be used.

It is basically the same as our previous analysis, but we did not think about the configuration of XML.

In the second paragraph, he begins to explain the problem from a documentation point of view.

Let's take a look at the Javadoc description on RollbackRuleAttribute.

There is a "NB" here, not the awesomeness we often say, but an abbreviation:

You see, I learned another English knowledge that I can't use.

Let's move on, focusing mainly on the two sentences I underlined.

The first sentence says: "Exception" can match almost any rule, and may hide other rules, due to the contains() method used. But the full-path string "java.lang.Exception" has a much smaller matching range.

The second sentence says: Because of the contains() method used, the full path name of the class is not required for unusual exception names such as "BaseBusinessException".

Therefore, what he wants to express in the second paragraph is: we have already said in the document, for matching rules, we must think carefully and be very careful:

u1s1, he did write it, but do you think you can read it?

The third paragraph is very simple:

When you see its subclasses, and its nested classes. , you know that this is the part we mentioned in the "Foreshadowing" section earlier.

So now you know why I paved the way for you?

If I don't give you a wave, you suddenly see the word nested classes of an inner class, do you think you can react?

You have to always trust my writing structure.

Well, now look at the other sentence I marked, the translation is to say: In the current implementation, the last sentence is not followed.

The "last sentence" here refers to the last sentence of the Javadoc of RollbackRuleAttribute, which is the sentence ... its subclasses, and its nested classes .

Of course not.

The two exceptions in the Demo I wrote are neither the relationship between the subclass and the parent class nor the relationship between the inner class.

So I'm wondering: there is no relationship, or conflict, between this Javadoc and my question. As I said earlier, I think there is nothing wrong with this part of the Javadoc. If you want to solve this problem from the perspective of modifying the document, you should not mention subclasses, inner classes, etc., you should start a new line entirely.

But how to solve it, he did not immediately express his position, but put this issue in the Triage Queue:

Queue, queue, you must know it.

What is Triage?

I didn't know either, so I also learned a new word:

That is to say, the official put this issue into a queue of "to be classified", which means that he has understood the problem, but how to solve it is still inconclusive and needs to be discussed.

A day later, this old man came to express his position again, and began to "jump horizontally":

He said he thought about it again and needed to correct what he said earlier: the Javadoc for the RollbackRuleAttribute(Class) constructor is mostly correct, which means it's basically fine. What needs to be improved is the description of the rollback rules.

Anyway, he still wants to fix this problem from a documentation point of view.

But it explained my previous doubts: even if this problem is solved from the perspective of modifying the document, it should not involve subclasses, inner classes, etc., it should be a completely new line.

His "rollback rule" here is "start a new line".

Then, he circulated the status of the task:

Moved from "To be classified" to the "Documents" tab.

Then said that it will be fixed in the 5.3.17 milestone version:

At the same time, the title of the issues was modified again:

Transaction rollback rules may result in unintentional matches for similarly named exceptions
Transaction rollback rules may result in unintentional matches for similarly named exceptions

In fact, if you let me deal with this problem, I will probably start from the perspective of documentation, and at most add a reminder log, after all, this is caused by your irregular use.

And my document has already stated that there is a "pit". You didn't see it and stepped into it. No wonder I am.

But after expressing my opinion with this reader, he put forward a different view:

He feels that most users do not pay attention to the log, and advocates the way of throwing exceptions for strong reminders:

So he expressed his opinion on issues:

He felt that more precise matching rules were needed, and most people didn't read the documentation.

Then the official adopted his opinion and moved the requirement to the milestone version 6.0.0-M3 to implement:

His specific reply is as follows:

He said: Dude, I agree with your point about "the need for more precise matching rules".

We will fix the documentation description for 5.3.x.

Then in version 6.0, we'll improve a version of the code.

Specifically this:

• If the exception mode is provided as a string. For example, in XML configuration or via @Transactional(rollbackForClassName = "example.CustomException") configuration, then the existing contains() logic will continue to be used.
• If a concrete exception type is provided as a class reference. For example, with @Transactional(rollbackFor = example.CustomException.class), there will be new logic. It takes full advantage of the type information provided on the configuration, thus avoiding an accidental match with example.CustomException2 when example.CustomException (without 2) is provided as the exception type.

The CustomException and CustomException2 he mentioned here are actually the code in his test case. Analogous to our previous AgeException and AgeExceptionOver18 these two exceptions.

Next, he reclassified the issue from the "document" type to the "enhancement" type:

Enhancement, is a fourth-level word, recite it, will test:

Indicates that this issue needs to be made more robust by modifying the code.

Then modified the title again:

For transaction rollback rules, exception type information should be used instead of pattern matching.

Originally, the story has already ended here, and when I wrote it here, I was ready to end it.

Don't be in a hurry to think about the ending, just sleep first and then talk about it.

result...

Wake up the next morning, he! again! Even! new! !

I have to add a piece of content.

final episode

When I woke up in the morning, as soon as I refreshed the page, I found that the official submitted the last issue for this issue:

The title of this issue is finally fixed as:

Support type-safe transaction rollback rules
Support type-safe transaction rollback rules

The corresponding code submission link this time is as follows:

https://github.com/spring-projects/spring-framework/commit/c1033dbfb3609f3b3fe002d7b582b3302620c05a

There is a long paragraph in it to describe the background of this submission, but it's basically a summary of what I've written before:

Combined with what I wrote earlier, I will translate the translation for you:

First of all, I think it is to create a new concept in the transaction module: type-safe rollback rules, type-safe rollback rules.

Before this commit, there is only one transaction rollback mechanism, Pattern-based rollback rules, which are based on matching pattern rollback rules.

The official said that the rollback rule based on the matching pattern will bring three unexpected matching situations:

• Exception classes with the same name in different packages are accidentally matched. For example, both example.client.WebException and example.server.WebException will match the "WebException" pattern.
• There are similarly named exceptions in the same package, where similar is when a given exception name begins with the name of another exception. For example: example.BusinessException and example.BusinessExceptionWithDetails both match the "example.BusinessException" pattern.
• Nested exceptions, that is, when an exception class is declared inside another exception class. For example: example.BusinessException and example.BusinessException$NestedException will both match "example.BusinessException".

The first one has nothing to say, please use the full path name to avoid it.

The second is the example in our article, which needs to be solved by modifying the code.

The third inner class situation I also foreshadowed before. But the solution at the time was to just add the corresponding description in the documentation.

But now, look what he had to say:

This commit prevents unexpected matches in the latter two cases. This means that this commit fixes not only our problem, but also the problem with inner classes.

So how to fix it?

The first is to add an exceptionType field to the RollbackRuleAttribute class:

Then assign it in the constructor:

The core code becomes like this:

When the exceptionType field, that is, the full path name is not empty, use the equals() method, that is, type-safe rollback rules. Otherwise, use the contains() method, which is Pattern-based rollback rules.

Also, a lot of the descriptions on the Javadoc have changed, so I won't list them all, but I strongly recommend you check out this commit for yourself.

I only take out one change in particular to show you:

Removed "inner class" and changed it to "type safety".

So far, the problem is solved.

But in XML or the rollbackForClassName attribute in the @Transactional annotation, that is, when using the matching mode, there will still be unexpected matching situations.

Official: Anyway, I made it clear on the document. If you still step on the pit, then it's not my fault?

Finally, one more thing about programming conventions.

You think this time the problem is entirely caused by the fact that you have two exception class names like this:

AgeException
AgeExceptionOver18

For exception classes, we all agree that the requirement must end with "Exception".

Including the Alibaba Java Development Manual, this is also specifically mentioned in the naming style, and it is a mandatory requirement:

So, if we all abide by this rule, everyone will be fine.

So, what does this story tell us in the end?

It tells us...

It tells us that the rules are meant to be broken. If you don't break the rules, you will never step on this pit, and you will not push Spring's changes.

Breaking the rules is one small step for you, but one giant leap for the open source world.

So, brothers, irons, don't stick to the rules, break the...

Finally, the article was first published on the public account [why technology], welcome to pay attention and receive the latest articles as soon as possible.