Reorganise database handling

A248 commented 3 years ago

This is, hopefully, the start of improvements for the PreciousStones backend.

I don't know much about PreciousStones's features, but there are a lot of improvements I can still make. I expect we can collaborate on how the database should be ultimately structured.

A248 commented 3 years ago

I plan to bust out the table creation to its own class. StorageManager is just too crowded.

A248 commented 3 years ago

I presume it's fine to remove the "Bukkit 1.7.5 UUID updates". No one will be upgrading from pre-1.7.5 PreciousStones.

A248 commented 3 years ago

This PR brings proper connection pooling, best practices with JDBC, parameterised statements to prevent SQL injection, reduced code duplication, and likely improved query performance through its connecting pooling and reduced contention.

Throughout all of my changes I have adhered to the original behaviour as much as possible. Some differences include narrowing the scope of 'synchronized' blocks when executing queries (as PreciousStones frequently synchronises in this context), removing useless try-catch Exception blocks, and avoiding fetching generated keys when not required. In some cases PreciousStones was computing update counts manually in a roundabout fashion, so I changed this to use the actual update count from JDBC. Also, I fixed a certain bug in processUnbreakable with a map retrieval.

When going through all of StorageManager, I noticed other problems.

First, StorageManager does occasional Bukkit API calls while it is getting the results of a query. One glaring example is in the pair of methods 'retrieveClearTranslocation' and 'retrieveTranslocation'. This is a serious issue no matter how you look at it: if the query is done on the main thread, it is thread safe, but can stall the server for a considerable amount of time. However, if this query ran asynchronously, it could not call the Bukkit API without violating thread safety. The only possibility I see is if this query is only run at startup, in which case it can be run synchronously.

Additionally, I noticed some details in these methods:

existsTranslocatior (changed to existsTranslocatior0 in this PR)
appliedTranslocationCount
unappliedTranslocationCount
totalTranslocationCount
existsFieldWithName
existsTranslocationDataWithName
MySQLCore(2)#getDataType

In each stated, the returned result is re-assigned inside a while loop iterating the result set. This means only the last row of the result set is used as the result of the method, since the other values are discarded. With this in mind, I wonder why the code is not made differently. ResultSet.last() would be an equivalent alternative in any case, and using 'if' would be appropriate when there is only 1 row. Thus, I would encourage you to look at this and ensure that the original code is the desired behaviour.

xtomyserrax commented 3 years ago

Hey, thank you for all the time you spent on this. I will try to do some checkins in my test server to see if things are working fine.

And let @NathanWolf to review this code as you know I have 0 knowledge of DB.

About translocators, they are actually not working because of this #8 , basicly as far as I could understand, its trying to set a location which is already set.

Thank you again for all the work you did, many servers will be pleased by this entire revamp.

xtomyserrax commented 3 years ago

Hey so I did some testing and when I start my 1.16.3 server I get this error: https://pastebin.com/BMfVnxTH

Here is the DB file Im using (which is from latest version): https://gofile.io/d/2GrfGX

When I log in I have this error: https://pastebin.com/HVm4zYct

This error when placing a field: https://pastebin.com/VbwidXHL (but I didnt find any error when breaking it and its deleted correctly from the DB)

And I found this error when I try to change the name of a field: https://pastebin.com/ZGtskUWn

I also tried to run the plugin with a fresh new DB but this errors also happen. Thank you for the awesome work you did and let me know if I can be useful for some help! @A248

A248 commented 3 years ago

The first error was a typo I made by forgetting to remove an apostrophe in a query, so that's been fixed.

The second problem was due to my misreading something in the original PreciousStones code. I looked back, and understood that when PreciousStones is using SqLite, the plugin tries to make up for the lack of syntax. However, PreciousStone's workaround has a different effect from the original MySQL syntax! Therefore the original code is itself surely bugged - see here. For now I have kept this as-is, but let me know what you think and want to do about it.

A248 commented 3 years ago

Also @xtomyserrax , you have been very helpful already! I especially appreciate you finding multiple bugs at once as that makes it much easier to fix them in one fell swoop of looking at the code. Although I am able to fire up a server if I really need to, your willingness to test these changes makes this a lot faster.

xtomyserrax commented 3 years ago

Hey, dont thank me, thank you for taking this time. What you did is just amazingly crazy and awesome!

First error was fixed. Now when I join I get this new one: https://pastebin.com/82vTN2Ge

And this quit error: https://pastebin.com/uhGYWBNW (seems related with the join one though)

/ps setname, /ps setowner and /ps migrate now have this error: https://pastebin.com/QybsLUUb and https://pastebin.com/87JvwtcC and https://pastebin.com/g7CczDib (I also noticed this same error with more commands so I wont keep adding them as the stacktrace looks the same)

/ps setradius has this error: https://pastebin.com/JnfCnM72

I havent noticed any errors with placing and breaking fields.

And yeah sadly there are some really old methods which are buggy. If you want to change stuff to improve it, then you are more than welcome (of course if it doesnt break other stuff). For example, moving from MaterialID to MaterialType would be amazing for fields but we would need to make a migration system for that #78

Let me know when you fix this issues and I keep testing. Thank you again!

A248 commented 3 years ago

Good

The join and quit error are the same bug (updatePlayer0 had an extra 'WHERE'). That should be fixed now.
The second problem should also be solved.
As for the last problem, I mimicked what the original PreciousStones did in this regard, so although I know clearly what the issue is - the query attempts to set values in columns which don't exist in pstone_fields - I'm unsure why this wasn't showing up before. I have implemented a fix by not setting these column value when the table doesn't have them.

xtomyserrax commented 3 years ago

Hey, sorry I couldnt test this earlier. This is what I found:

Join error: https://pastebin.com/vLKzq15u
Quit error: https://pastebin.com/qNQ3s45j (seems to be the same as join one)
/ps migrate and /ps setowner error: https://pastebin.com/yqmqn5Ed (seems to be the same as join one)
No more errors with setradius, setname, etc.

So for now I just only found this error which seems to be related with many commands that involve players. Ill keep testing after this fix, many commands are working perfectly now.

Thanks!

A248 commented 3 years ago

No issue, I have time. The latest error should be solved now.

xtomyserrax commented 3 years ago

Heeey, for now I have seen none errors. I will keep testing this to see if there is a hidden one but seems stable. If Nathan wants to check the code he is free to do it!

Thank you!

xtomyserrax commented 3 years ago

I have done more testing and I havent noticed any errors. This revamp seems to work amazingly well!

elBukkit / PreciousStones

Reorganise database handling #74