Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Support evolution on MySQL 5.6 #67

Closed
wants to merge 1 commit into from
Closed

Support evolution on MySQL 5.6 #67

wants to merge 1 commit into from

Conversation

epugh
Copy link
Contributor

@epugh epugh commented Mar 11, 2021

Mysql 5.6 didn't like a index being made with keys longer setting than 767. So this supports that change.

@epugh
Copy link
Contributor Author

epugh commented Mar 16, 2021

Hi all, I'd love to get some feedback on this change, as we'd like to use a released version of SMUI with this change made.

@mkr
Copy link
Collaborator

mkr commented Mar 17, 2021

Thanks for the patch. It seems that we support only MySQL 5.7+ for now as we have indexes on VARCHAR columns wider than 767 chars (a limitation lifted in 5.7). We can't really change old Play evolutions because this would potentially lead to data loss for existing users. To add support for older versions of MySQL or new systems such as SQL Server (as in #66) with incompatible DDL scripts I see these options:

  1. Add a distinct set of migrations in a folder in /conf/evolutions/mysql-pre-57 and /conf/evolutions/sqlserver which reflect the mainline migrations but adjust the tables where needed. Explicitly dispatch to these migrations based on some application setting.
  2. New major. We provide a new 1.sql initial DB evolution that describes the DB in a way that is compatible with all DBs we want to support. For existing customers we also provide a script to run on the existing DB which alters/removes the evolution history and data tables to reflect the expected state.
  3. New major and automated transition. Same as 2 but with automated script execution by SMUI.

@pbartusch, @renekrie Opinions?

@epugh
Copy link
Contributor Author

epugh commented Mar 17, 2021

I may have goofed with git.. I wanted to just cut a quick branch that supports mysql and mssqlserver on Java 1.8, to get through a deploy with two clients.. I thought the commits would all be on https://github.com/o19s/smui/tree/support_multiple_db_types_on_java18. Not sure how to unwind things!

@pbartusch
Copy link
Collaborator

Hi @epugh ,

I very much appreciate the contribution for a broader database support for SMUI.
As @mkr pointed out (#67 (comment)) , the suggested implementation would cause this SMUI version to be incompatible with previous SMUI installations.

But I understand SMUI's data model could be improved at this point to offer broader support for DBMSs.

My 2 cents on the topic:

Regarding MySQL < 5.7: SMUI should not encourage supporting old DBMSs , that might also pose a security risk to their users (as I found some CVEs , that seem related to that MySQL version).

Regarding MS SQL Server: This seems to be a legit new requirement for SMUI. This comes with the need to change the database of SMUI.

As for the options @mkr highlighted (#67 (comment)) , I think:

  1. Will be hard to maintain , as every future add-on to the database structure needs to be synch'ed with all configurations (Play evolutions).
  2. Might be possible , but seems hard to be reflected in an automated test case.
  3. Seems the best option to me here , but comes with a bit of effort especially for the test automation.

As before this PR , my strategy regarding persistence in SMUI was mainly driven by the idea, to further constraint the support of DBMSs (e.g. Postgres-only) to avoid compatibility issues often encountered while extending SMUI in the past. This PR heads into the opposite direction , where you seem to see a market need for that.

I'd like to learn more about that , to reinforce a strategic path for this question (as for extending the PR according to one of the options 2. or 3.).

What do you think?

@epugh
Copy link
Contributor Author

epugh commented Mar 20, 2021

I am very much conflicted....! If I cast my mind back to when I made my very first small contribution to SMUI, I too had the "we have too many database options". Indeed I argued the same as you @pbartusch on we should treat the database as a implementation detail. Especially in Docker friendly environments.

I'm a bit stuck though, and only because I have two clients at the same time who don't use Docker in production (one doesn't even know what Docker is), and neither uses mysql5.7. The first is on mysql 5.6, the other is a .net shop so MS SQL Server.

I also am starting to better understand that the database is important from the sense that it represents the accumulated knowledge of all the people and they don't want to risk losing it, and many ecomm tech teams are stretched for time and resources, so they want to use the database they already know and understand. I dread the day that a team I worked with last year brings me back in and they want DB2 support ;-). Big WCS user ;-)

I do think that regardless of what we may do, we need to solve the integration testing issue. I'd love to see the testcontainer work that @tboeghk made part of this project.

I could keep trying approach 1 in my o19s/smui branch, and seeing how viable the multiple database thing really is. For example, I found I need to change the code parsing the database response for MS SQL Server. If those changes to the app layer aren't pretty simple (and work across all our databases) then the whole effort on MS SQL Server might be for nought!

@pbartusch
Copy link
Collaborator

pbartusch commented Mar 20, 2021

alright @epugh .

I could keep trying approach 1 in my o19s/smui branch, and seeing how viable the multiple database thing really is.

sounds like a fair temporary solution. just keep in mind, that there is a risk the branch will get outdated soon as SMUI development proceeds. so , for installations based on the branch it might become necessary to migrate the data anyway (which might lead to solution 2. or even 3. naturally - let's see ;-) )

I'd love to see the testcontainer work

me too , actually ... thanks for pointing that out. @tboeghk , if you could share that container or drop a link , that would be awesome!

for this PR , @epugh :

  • I will close it as for the compatibility issue.
  • I will open an issue for a broader database support - including DB2 of course ;-)

[...] seeing how viable the multiple database thing really is

let's channel further communication on this topic on the issue page (maybe we can introduce some kind of voting in the future).

@pbartusch
Copy link
Collaborator

pbartusch commented Mar 20, 2021

PR closed (without merging for compatibility issues, see above). Issue tracked as enhancement #69 .

@pbartusch pbartusch closed this Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants