ebean-orm
Add metadata to stored procedures for 3rd party libs
Hi @rbygrave!
You may remember the problem we, from the Play Framework, had in
- https://github.com/playframework/play-ebean/issues/166
The problem is that Play splits sql scripts by ; meaning that any stored procedures that usually contains ; in their body will be split into weird statements, making the sql script fail. We do have an escape mechanism by using two ;;, which avoids the string getting split and the double ;; will end up as a single ; in the final statement.
The problem is that when ebean (re)generates its ddl scripts it will override existing once, so each time people would have to replace ; with ;; which is annoying. Even worse, lots of people don't even know what's going on and just see an exception without having a clue why ebean generated ddl scripts don't apply with Play evolutions and just open issues like "ebean doesn't work", "play-ebean doesn' work", etc.
Now I think the solution to this problem is actually quite easy: If you could add meta data to your ddl scripts, like shown in this pull request, we could easily parse that meta data and make Play evolution ignore the semicolon within such a "meta data block". The naming of the meta data comments I came up with is currently:
-- !Play-Evolutions: KEEP SEMICOLONS START
-- !Play-Evolutions: KEEP SEMICOLONS END
but I am totally open for other naming.
What do you think about that? IMHO this shouldn't hurt ebean but 3rd party libraries would benefit from finally being able to correctly handle your ddl scripts.
Thanks!
Ohh, I hoped you already have a solution, but this PR is just an issue, with a code example, isn't it?
Just reading the title and the description I tought you would implement a new entrypoint for 3rd party libs in the ddl generation code to programatically put those BEGIN & END markers in the ddl payload.
(Just to note: I'm just a follower, I don't even use Ebean. 😅 But the solution you came up is very interesting. Subscribed to the issue.)
but this PR is just an issue, with a code example, isn't it?
No, this is not just an issue, it's also an actual pull request with the change required to make this work. The only thing that needs to be discussed is the naming of the "meta data".
Just reading the title and the description I tought you would implement a new entrypoint for 3rd party libs in the ddl generation code to programatically put those
BEGIN&ENDmarkers in the ddl payload.
No, this isn't necessary, my solution is much simpler and does its job perfectly.
(Just to note: I'm just a follower, I don't even use Ebean. 😅 But the solution you came up is very interesting. Subscribed to the issue.)
You come here from the Play Framework issue?
You come here from the Play Framework issue?
I was browsing the activity of the repo with OctoDroid and noticed that the lead developer of Play! Framework opened a PR with an interesting title. :slightly_smiling_face:
Well, I think there are 2 other alternatives to consider.
One option would be to just copy Ebean's DdlParser code - https://github.com/ebean-orm/ebean-ddl-runner/blob/main/src/main/java/io/ebean/ddlrunner/DdlParser.java#L55-L61 ... with the benefit that this would work with not just ebean migrations but any DDL scripts that people want to parse and execute.
A second option is to adjust the Play parser to support the "more standard" delimiters and I'm pretty sure there are only the 2 cases needed to be supported for identifying the start and end which is:
-
delimiter $$and$$[for MySql, MariaDB, Postgres etc] - contains
PROCEDUREandGO[for MS SQL Server]
So these approaches have some benefit as I see it. Have these options been considered?
The problem is that when ebean (re)generates its ddl scripts it will override existing once, so each time people would have to replace ; with ;; which is annoying.
I'm not sure if there is a misunderstanding here but Ebean has 2 types of DDL generation: (A) DB Migrations - https://ebean.io/docs/db-migrations/#generate (B) "create-all.sql" / "drop-all.sql" - https://ebean.io/docs/ddl-generation/#create-all
Ebean will regenerate the "create-all.sql" / "drop-all.sql" every time but to be clear these are never intended to be used for db migrations but instead for general local development running tests. People should not be treating these as migration scripts.
Ebean has a separate tool to generate db migrations (based on a "diff" of the model). This tool is generally explicitly run to generate the next db migration script (and model change). When we want to create a db migration we should use this tool and this does not re-generate or override any ddl scripts etc.
Play splits sql scripts by ;
To me that sounds like it does not support MS SQL Server GO based scripts.
I'm not sure if there is a misunderstanding here but Ebean has 2 types of DDL generation:
@rbygrave This is what we do currently: https://github.com/playframework/play-ebean/blob/main/play-ebean/src/main/java/play/db/ebean/EbeanDynamicEvolutions.java#L118-L137 Is that OK? (I did not write the original play-ebean plugin).
And how does your diff mechanism work? Is it that you read the schema from the database, and store that state somewhere, and now a dev modifies ebean models/annotations, that will become the new state and you diff that two states? Or, instead of reading the schema of the database, do you read sql files (the scripts that you generated before) and from that you know that original state?
Thanks!
@rbygrave One more thing: I do think about that our play evolutions script parser should eventually become smart enough to understand stored procedures. However, I want to make that bullet proof and will need some time.
Completely independent of that (play-)ebean problem we face, what I also want to do in general, is to add a config to Play where folks can define a start "label" and end "label", as sql comments, which makes the play evolutions script parser not split on semicolon for all semicolons that are between such two labels (sql comments).
Again, this is a more general issue for us and not directly related to play-ebean. I opened an issue for that with example code:
- https://github.com/playframework/playframework/issues/12968
Now... if you look at the diff of this pull request, would it be possible if you add such meta comments to you procedures, just like this PR? You could choose any comment "names" you desire, it just would have to be a "start" and and "end" label. The thing is, that would help us quite a lot right now, we have many people complaining and with this subtle addition to your procedures which, since they are comments, will not affect anyone, would help us a lot right now.
Would you consider merging my PR with a naming of the comments that you choose?
Would you consider merging my PR with a naming of the comments that you choose?
Just an idea, something like:
...
BEGIN
-- ebean-orm-sproc-body-start
...
-- ebean-orm-sproc-body-end
END
GO
@rbygrave So I updated the pr with -- ebean-orm-sproc-body-[start|end]. This is now agnostic to any framework, its just ebean meta data. What do you think?
This is what we do currently: ... Is that OK?
Well no in that it isn't an actual diff based migration generation.
how does your diff mechanism work?
It stores the "logical model" [which is database platform agnostic] in "model xml files". That is, when the migration is generated it generates the sql migration AND the "model xml".
The "diff" is computed by reading all the "model xml" and then comparing that against the current model [the current model based off the entity beans as they are currently]. With the "diff" it can then generated the database migration - the sql database migration PLUS the "model xml" that goes with it.
read the schema from the database
It doesn't need to read the database schema. [Weird historic fact, the pre-cursor to Ebean did in fact read the database meta data. It was more an "Active record" style persistence library with specific types and not an ORM per se]
add such meta comments to you procedures, just like this PR?
This seems to me like a band-aid. That is, it doesn't seem like actual diffs are managed and generated and there are details in running migrations like non-transactional sql statements like Postgres create index concurrently that also need to be detected and handled. Seems to me like longer term play-ebean folks could and probably should migrate to using the ebean tools for migration generation and migration running. Hmmm.
Lets change this to -- play-ebean-start and -- play-ebean-end ... in the sense that I think play-ebean should be the only consumer of these comments. Ideally at some future point we don't need these comments etc.
This may not be the ideal solution, but it would help play-ebean users for now until we come up with something better.
Lets change this to
-- play-ebean-startand-- play-ebean-end.
Changed, thanks!