Fix MySQL index length

[MikeAlhayek]

Fix #14338

@sebastienros @jtkech looks like we missed couple more indexes in 1.7.1.

I also enabled OrchardCore.ContentFields.Indexing.SQL for the BlogContext. Typically this is done in a recipe. But I did it directly using ShellFeatureManager as I did not want to update the blog recipe.

Will these tests run my MySQL server?

[sebastienros]

Haven't looked at the changes but maybe we could have a custom review to copy before running the tests that would enable everything. Other ideas?

[jtkech]

Haven't looked at the changes but maybe we could have a custom review to copy before running the tests that would enable everything. Other ideas?

We run recipes for tests, maybe we could use a recipe that contains quite all features just for testing.

[MikeAlhayek]

@jtkech Hummmm, tests after your last commits should have failed. But I am sure you know that.

Does these tests run on MySQL 8?

[jtkech]

Okay, the problem is that the Functional Tests - all Databases can only use existing recipes, and that Unit Tests only use Sqlite, we would need an Unit Tests - All Databases.

In the meantime for testing I will try to set the default for Unit Tests to Postgres.

[jtkech]

Can't create a service container on Windows, only on Unix. Not easy to create a postgres service directly on the machine without specifying explicitly a container as done in Functional Tests - all Databases.

Maybe I could get it working but unit tests was still not completed after 45 minutes ;) So anyway would not be a viable solution. But if it works maybe only a simple test to setup a tenant per database.

I will remove the added OC.ContentFields.Indexing.SQL feature, for now not useful here, Otherwise I found a simple way to enable all features in unit tests by adding the following in SiteStartup.

            .ConfigureServices(collection =>
            {
                ...
                collection.AddAllFeaturesDescriptor();
            })

For info, with all features enabled all unit tests passed successfully, recipes were only used for example to create contents but not to enable features as they were already all enabled.

The other strategy would be to use Functional Tests - all Databases and find a way to detect that we are running tests and then enable all features.

[jtkech]

So one idea would be to use an env variable OrchardCore__AddAllFeatures to tell that we want to enable all features because we are running functional tests. I started to try this scenario.

[jtkech]

Okay it begin to works by using an OrchardCore__AddAllFeatures env variable, by reverting one column size I could make Postgres failing in the Functional Tests - all Databases (can only be run manually).

But that's the same if I reuse the right size, maybe because there are still some too high sizes.

Anyway too much features including those using not well configured external services, so many log messages, so maybe enable all features but only those that have a DataMigration defined.

Then step by step try to find if some DataMigration still need to be updated, will see tomorrow.

[MikeAlhayek]

@jtkech I did another audit and found a missed index in the Taxonomy. I enabled the test to run with all the features enabled. Now everything seems to be passing.

Please confirm that the change I made in the ShellOrchardCoreBuilderExtensions are valid.

[MikeAlhayek]

@jtkech I think there is an issue with the tests. My last commit should have forced the tests to fail but they passed.

[jtkech]

@MikeAlhayek

The Unit Tests and Functional Tests in a PR github action only run on Sqlite, so any tests here are not relevant. Should use the All Databases tests that can only be run manually in github action.

This is in these tests that I set an env variable that we detect by code, this part is now working. Currently in my tests when we detect this variable we only add the OC.ContentFields.Indexing.SQL feature.

I did another audit and found a missed index in the Taxonomy

Yes I did it and reverted it because tests were still passing. The "problem" is that now when I revert some changes the All Databases tests still pass.

Just saw that #14338 is related to MySql and I was focusing on Postgres that from memory had a similar issue.

But I saw that for the All Databases tests the MySql image has been changed from mariadb:latest to mysql:8 which may allow higher sizes.

So maybe nothing to fix and just say to use another version ;)

But I will work on it this night, for example by re-using mariadb:latest for testing.

[MikeAlhayek]

@jtkech MySQL 8 docs says this

The index key prefix length limit is 3072 bytes for InnoDB tables that use DYNAMIC or COMPRESSED row format.

Anyway, I'll leave the testing changes for you to handle.

Thank you!

[sebastienros]

when we detect this variable we only add the OC.ContentFields.Indexing.SQL feature.

I think maintaining a custom recipe for functional tests might be simpler. This ENV sounds like a hack and might be an issue eventually.

[jtkech]

I think maintaining a custom recipe for functional tests might be simpler. This ENV sounds like a hack and might be an issue eventually.

Yes I thought about that but this dedicated recipe should not be suggested in a regular setup.

I will think about another solution.

For now I will still use this hack to add some features because I want to see again the All Databases tests failing or not when a size is > 3072 or not, maybe be by re-using mariadb:latest just for testing.

[sebastienros]

Yes I thought about that but this dedicated recipe should not be suggested in a regular setup.

This could be one in the function tests folder that is copied by the test script.

maybe be by re-using mariadb:latest just for testing.

We can add it too, I had to because it was failing during the container initialization (not an OC issue). this can be tested in a branch and start the functional tests manually on this branch.

[jtkech]

Okay will look at it

[jtkech]

Okay, by adding a feature with a size > 3072 I now am able to make the All Databases failing or not.

I now have an objective way to check all feature migrations Create() methods.

For info this is only MySql which fails if a size > 3072, not Postgres.

[jtkech]

I didn't need to re-use mariadb:latest, the problem was that I was setting the env variable only for Postgres.

[jtkech]

For info multiple booleans share at least the same 2 bytes, maybe the same 4 bytes integer.

But it's okay to keep that as a margin.

[jtkech]

Okay all features having migrations pass, only OC.AdminDashboard makes the All Database tests failing, its migrations uses a migration recipe file, I will check locally what happens.

[jtkech]

Okay it fails because it expects Welcome to Orchard and it finds something related to Manage Dashboard.

[jtkech]

Okay, all works fine.

I will remove the usage of an ENV VARIABLE to add more features.

So the only remaining thing will be to have a dedicated testing recipe, for now the All Databases tests only simulates an user filling the regular setup screen that only provides regular recipes.

[jtkech]

Okay Cypress builds and also uses the OC.Tests project (referencing the web app).

So looks like we can define a dedicated recipe that could be used not only in Unit Tests but also in Functional tests.

Edited: In fact it builds (if not yet done) and runs the web app itself.

[jtkech]

Okay, all works now, yes I needed to copy the dedicated migrations recipe in test-runner.js.

• So when the PR github action runs, because the functional tests are executed this recipe (that adds features having database migrations) will also be used but only on Sqlite.

• When running manually the All Databases github action, the same happens but on all databases.

Just need to check again that the All Databases fails if we increase the size of an index related to a feature included in the dedicated migrations recipe.

Edited: Okay done, I first did a recipe based on the SaaS one, but then I was seeing migration errors but the test itself was not failing because there is no content to request. So I updated it to be like the Blog but then it was not finding somme assets, so I had to cleanup it, maybe some other cleanups could be done.

That said all is still working and the All Databases fails if an index size is too high.

[jtkech]

@sebastienros

This could be one in the function tests folder that is copied by the test script.

You were right and this is what I did ;)

[MikeAlhayek]

@jtkech are you done making changes here? It is a merge ready?

[sebastienros]

If it's for 1.7.2 then please target release/1.7, this will make things easier. I can change it myself but I'd prefer someone else to do it for the experience.

[MikeAlhayek]

@sebastienros changing the target or merging it into main first, cherry pick it into 1.7 as we did with 1.7.1? I think we can change the target from the Github UI if that is what you want to do this time.

Maybe you want to make the change to release/1.7 then merge release/1.7 back into main to reduce the chance of conflicts.

[MikeAlhayek]

@sebastienros changing the target causes way to many files to be changed. Do we need to close this one and open a new one?

[sebastienros]

Let me fix your branch, and then you can try again and it won't change all these files. I am rebasing your branch on release/1.7, like it should have started. Check back in a few minutes.

[jtkech]

Yes I'm done and it could be merged if it is urgent.

Otherwise maybe a better name for the migrations.recipe.json file and maybe some cleanups, but can be done afterwards in a separate PR.

Also for now I created a Recipes folder under the web app, maybe better to be only done by the runner script, but makes sense to have such a folder, but if so would need to be published like we do for the Localization folder.

[sebastienros]

Done. It's not merged yet, you can continue working on it.

@MikeAlhayek I am seeing this in the history, unless I missed something, main should never be merged to release branches, only the other way. But this is a working branch so there is no harm right now.

[MikeAlhayek]

@jtkech maybe a good time to merge and release this during the meeting today. If you have last minute cleanup, go for it meanwhile.

@sebastienros thanks for fixing that. I agree main should never be merged into a release branch. Not sure where that history item is coming from. Maybe because the original branch was based on main.

[jtkech]

Hmm, I think it's better that the runner script creates the Recipes folder, the nodejs copy doesn't auto creates it, that's why I created it manually for testing.

But easy to do, so if you can wait a little I will do it this night, but need to be done carefully and need to do a last test.

[MikeAlhayek]

@jtkech we can wait. If we don't release it today, we can release it tomorrow/Thursday. But, when you are done, please let me know so we know when to proceed. You can also merge it once you are done with it.

[jtkech]

Okay, almost done locally but need to leave

[jtkech]

Okay done, I'm starting to cleanup the migrations recipe to only what is needed.

[jtkech]

Okay all done, I let you merge it if other things are ready.

For info to see the Functional Test - all Databases results you need to go there https://github.com/OrchardCMS/OrchardCore/actions/workflows/functional_all_db.yml where you can run the workflow from a given branch.

[MikeAlhayek]

@jtkech thanks a lot for all the hard work on this PR! I'll prep the release and release 1.7.2 soon.