[ci] Add missing transient per sonarqube

[hazendaz]

No description provided.

[FrantaM]

Caused by: java.lang.NullPointerException: null
        at org.apache.ibatis.cache.CacheKey.equals(CacheKey.java:99)
        at net.sf.ehcache.store.chm.SelectableConcurrentHashMap$Segment.get(SelectableConcurrentHashMap.java:876)
        at net.sf.ehcache.store.chm.SelectableConcurrentHashMap.get(SelectableConcurrentHashMap.java:360)
        at net.sf.ehcache.store.MemoryStore.get(MemoryStore.java:300)
        at net.sf.ehcache.store.MemoryOnlyStore.get(MemoryOnlyStore.java:107)
        at net.sf.ehcache.Cache.searchInStoreWithoutStats(Cache.java:2070)
        at net.sf.ehcache.Cache.get(Cache.java:1590)
        at cz.sa.ares.database.mybatis.cache.EhcacheCache.getObject(EhcacheCache.java:57)
        at org.apache.ibatis.cache.decorators.LoggingCache.getObject(LoggingCache.java:57)
        at org.apache.ibatis.cache.decorators.TransactionalCache.getObject(TransactionalCache.java:68)
        at org.apache.ibatis.cache.TransactionalCacheManager.getObject(TransactionalCacheManager.java:35)
        at org.apache.ibatis.executor.CachingExecutor.query(CachingExecutor.java:101)
        at org.apache.ibatis.executor.CachingExecutor.query(CachingExecutor.java:83)
        at org.apache.ibatis.session.defaults.DefaultSqlSession.selectList(DefaultSqlSession.java:148)
        ... 61 common frames omitted

[FrantaM]

Note that only making the field transient is just plain wrong. You lose the updateList on serialization (replication, disk store) and make the cache effectively memory-only.

[hazendaz]

@FrantaM

Transient is simply a marker to java. It is required when an object is NOT serializable. The object would fail either way if that data element is not managed properly. It would get a serialization error without transient if it was attempted to be serialized because it does not implement serialization (unless that changed since April 23rd). With it, it doesn't try to serialize thus the null pointer that you see upon object being restored for use. Can you please open up a new issue if you haven't already that points back to this change and/or take your stack trace and place on the new issue. We will look into it.

Thanks,

Jeremy

[FrantaM]

It is required when an object is NOT serializable.

So why did you add it? The object in that field is ArrayList and ArrayList is serializable just fine.

[harawata]

Hi @FrantaM ,

When you open a new issue, could you provide a link to a test case or example project, please?
We should add a test case to avoid regression.

Thank you,
Iwao

[hazendaz]

@FrantaM

That is true if it were using ArrayList but it's using the interface which itself is not serializable. Neither is Object.

Why I would have added it? because one of the various checkers (findbugs, checkstyles, or pmd and/or sonar stated to do so and there was a presumption this was being handled without errors thus no need to add in the serialization logic necessary to handle the transient).

I feel I should now test this to confirm our scanners are correct and any assumptions are as well.

Serializable by definition of a class requires that each element of the class is also serializable otherwise it's not actually serializable.

For reference, what version are you using of mybatis? I belive this was introduced in 3.4.5 which just released. I'm assuming you are then using 3.4.5 but wanted to be sure. In that case, could you try dropping back to 3.4.4 and see if you can confirm if it is still an issue there?

Now looking closer at your stack trace, it's not failing on the class itself but rather the object passed in for comparison.

So I'm wondering if this is really just exposing another issue.

for (int i = 0; i < updateList.size(); i++) {
  Object thisObject = updateList.get(i);
  Object thatObject = cacheKey.updateList.get(i);  --> This line.  Nothing was done to ensure it even had 'i' at this point.  What if it doesn't.  Is updateList really null or the I its attempting to pull not present?
  if (!ArrayUtil.equals(thisObject, thatObject)) {
    return false;
  }
}

Is it possible the equals is implemented incorrectly and we have now exposed it? A good test case can serialize and confirm the issue but would need to know a bit more on how your use case is using that. I can look into building a serialization of this class to see if I can flush this out.

[hazendaz]

I happened to have a project that tests for serialization that I thought would help here. However, it only goes as far as to write the object and that isn't enough ot prove serialization if some or anything is serializable. Obvious outcome there is that this helped me see my testing project 'javabean-tester' isn't solid enough to ensure serialization is perfect. Since its just a marker to java, it simply ignores what isn't serializable anyways. Anyway in my test, I addeda truely non serializable object I made up. Running java bean tester using serialization write, it succeeds. If I run my dummy class by itself it fails. Contents of the dummy class themselves are serializable. Thus this is going to take me a bit longer to write something up that is more solid. I'm going to have to perform both the write then read to see what the data contains. I suspect null in both cases but will take me a few to confirm.

[hazendaz]

confirmed. Scanners are wrong. This was a bug. I'll review all my other work. the 'transient' needs removed.

[hazendaz]

Of course it all matters on what is inside the list.

If string, the transient of causes it to be not saved and ends up null.
If object, it won't serialize at all.

So scanners are half right. Sonarlint is the problem in this case.

@harawata

This appears to be a breaking change that needs pretty immediate attention. Should we do a 3.4.6 release?

[hazendaz]

@FrantaM @harawata

I have raised PR #1084 to fix this issue with enough testing around it that it never occurs again.

[FrantaM]

That is true if it were using ArrayList but it's using the interface which itself is not serializable. Neither is Object.

Regarding serialization, the type of the field does not matter at all. You don't really need to tell me how serialization works.

For reference, what version are you using of mybatis?

3.4.5 and I cannot really go back because 3.4.4 contains another serious issue (#988).

Is it possible the equals is implemented incorrectly and we have now exposed it?

No. Marking updateList as transient made it nullable - cacheKey.updateList can by null and you can't call get() on null.

Of course it all matters on what is inside the list.

Note that the only non-serializable thing that may end up in updateList are mapper parameters. But this can by easily avoided by the users or they can just use memory-only cache or not use cache at all.

[hazendaz]

@FrantaM Thanks for feedback. I think we are all good now. Thanks for catching this so quickly as well. Sonarlint is pretty strict on stating it is an issue and after all the work I did to test it, it's not so clear. The use-case in which it is based on real usage as you point out is pretty weak at best. This is the second or third issue I've seen with the new sonarlint that ultimately is flat out wrong. Again thanks and thanks for letting me know going back is not an option. I'm waiting on feadback from @harawata and expect this needs emergency release tonight. I've got a head out for a bit but will check back in a few hours.

[harawata]

Thank you @FrantaM and @hazendaz !

So, to describe the issue from the user's perspective, is the following accurate?

Caches that perform serialization didn't function correctly.

Any correction or rewrite would be appreciated.

[hazendaz]

Code is on master now. I merged the initial fix back to how it was with thorough tests and @FrantaM helped me cleanup from actually writing out a file in the process. I think generally speaking serialization is up to the jvm needs and not usually controlled on our parts. So this is probably a horrible regression as any to have leaked onto the code base. However, I don't know the full ramifications of doing nothing at the moment. I am surprised this didn't pop up sooner during various testing. I sort of think it would be good to release 3.4.6 but not sure if we want to wait a few days to see if any other regressions exist after this overall release. The release feels big to me anyways that maybe a few days is worth the wait.

@FrantaM @harawata Thoughts on waiting?

@harawata That basically sums it up. The issue here is that serialization still occurred but the 'updateList' is simply ignored due to the transient I had added per sonarlint insistence resulting in it being null. While the insistence is mostly true, it's really only true if the actual objects being written are not themselves serializable. So string for example would serialize with no problems. A non serializable object or Object as defined would fail to serialize. In this case, good user usage would not have resulted in a serialization problem to start with. However, since I added transient, it causes the object to become null as it is never written and not there to retrieve. A better fix would be to know something better than object to store in the first place which I think would avoid ever having this situation. However, not sure if anything being mapper is what is stored here. I'm mostly speaking for me here as I don't know all use-cases of a lot of the overall code.

[FrantaM]

Caches that perform serialization didn't function correctly.

Edit: oh, you mean to describe this bug? Then yes. Or more precisely caches (cache elements) that are deserialized because you won't hit this until then.
So if you only store the cache to disk on application exit (or web context undeploy) and load it at start you won't encounter this until you restart the app. But when you do, you're screwed.

Not really. More like: If you define cache in your mapper, the cache can be serialized only when mapped statements parameters can be serialized.

Thoughts on waiting?

Well this thing rules out cache replication and loading serialized cache (e.g. from disk) without resorting to some dark voodoo reflection as I did. But I know mybatis internals so I can afford it. I would definitely not wait too much before releasing this.

[harawata]

Not really. More like: If you define cache in your mapper, the cache can be serialized only when mapped statements parameters can be serialized.

Thank you for the explanation.
I thought that caches could not be serialized at all if transient was specified.
(I was thinking about what to write in 3.4.6 release note, by the way)

Thoughts on waiting?

As @FrantaM can afford it, I think it's OK.

[FrantaM]

I thought that caches could not be serialized at all if transient was specified.

Caches can be serialized, but the serialized form is useless. And when you try to deserialize & use it you'll end up with NPE.

[harawata]

Ah, yes. That was what I thought. Thank you for the clarification!