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

Change the checksum computation to be before parameter replacement #91

Closed
rbygrave opened this issue Aug 5, 2021 · 6 comments · Fixed by #142
Closed

Change the checksum computation to be before parameter replacement #91

rbygrave opened this issue Aug 5, 2021 · 6 comments · Fixed by #142

Comments

@rbygrave
Copy link
Member

rbygrave commented Aug 5, 2021

So, I have the plan that migration generation can generate idx<platform>.migrations files. These files contain the checksum and migration resource (this actually already exists in 12.11.0 in ebean).

The problem is that this checksum is based on the sql file resources as they exist on the file system. The checksum that ebean-migration uses is computed after the parameter expression replacement ( like ${...} ).

Ideally ebean-migration changes such that the checksum is computed before the parameter expression and in this way we can use idx<platform>.migrations files to:

  • avoid classpath scanning search for all the migrations
  • compute the checksum at that point

That is, especially in CI/CD environment a lot of application starts happen with no actual migrations to run. ebean-migration does a pretty decent amount of work which gets more and more as the number of database migrations grows. We can use the idx<platform>.migrations files to make this case a lot faster and more efficient which will be pretty handy when there are lots of DB migrations and/or those migration files are large.

The challenge is to more the computation of checksum (which will change the checksum value for migrations that have property evaluation / replacement) and do this transparently without breaking peoples existing migrations.

My current plan is to use the 0 migration which has a checksum value of 0. Changing that to 1 to indicate that the new checksum is being used - when that 0 migration has a checksum value of 0 that would indicate that it uses the old checksum compute.

So we need to be careful here but we should get a good long term payoff in dealing with lots of migrations and or large migration files.

FYI @rPraml

@rPraml
Copy link
Contributor

rPraml commented Dec 2, 2021

@rbygrave FYI:

It seems, that ebean 12 does already compute the checksum before parameter replacement, because all migrations with parameters has checksum errors now. (but this issue is still open and I did not find an issue/commit that's pointing out the change.)

This is not a big deal, for us, because only a few migrations are affected and we added as it could be easily fixed by specifying them in the ebean.migration.patchResetChecksumOn=1.234 property.

In other words: From our perspective, we can live with that incompatibility.

cheers
Roland

@rbygrave
Copy link
Member Author

rbygrave commented Dec 7, 2021

It seems, that ebean 12 does already compute the checksum before parameter replacement, because all migrations with parameters has checksum errors now

That should not be the case no. I have not yet made that change, so I'm unsure why you saw checksum errors - odd?

@nPraml
Copy link

nPraml commented Jan 11, 2022

@rbygrave FYI:
our problem was first of all with the special characters e.g. 'ü' in migration scripts.
Maven in cmd and Eclipse computed different checksums.
Our fix would be in PR ebean-orm/ebean#2485

@jonasPoehler @rPraml

@rPraml
Copy link
Contributor

rPraml commented Apr 20, 2022

@rbygrave do you already know, when/if you implement this feature.
We ran into an issue, where we do some parameter replacement in the scripts and the checksum changes as soon the parameter changes.

In our case the parameter was used in tablespace definition (@Tablespace("${tablespace}")) and we need to change that. So many scripts were affected by checksum mismatch.

@rbygrave
Copy link
Member Author

I have not yet made this change. I'm keen to do it but it keeps slipping ...

@rPraml
Copy link
Contributor

rPraml commented Oct 31, 2023

Hello Rob, I think the change in #138 means there is an incompatible change to the use of checksums in combination with parameters.
If you have time, please read the comments in #138 - Do you share my opinion or am I overlooking something?

Roland

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 a pull request may close this issue.

3 participants