-
-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add query bean support #64
base: main
Are you sure you want to change the base?
Conversation
val querySettings = Seq( | ||
playEbeanQueryGenerate := false, | ||
playEbeanQueryEnhance := false, | ||
playEbeanQueryDestDirectory := "app", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should go to SBT's classes_managed
directory instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it needs to go into a managed source directory, assuming it's a Java file. There's actually a better way to configure directories. Take a look at the Twirl sbt plugin to see how it does it:
https://github.com/playframework/twirl/blob/master/sbt-twirl/src/main/scala/play/twirl/sbt/SbtTwirl.scala.
Concretely here are some steps:
- Create a new task called
playEbeanQuery
. Call this task fromebeanEnhance
ifplayEbeanQueryGenerate
is created. - The task should read from
sources in playEbeanQuery
and output totarget in playEbeanQuery
. - Add
playEbeanQuery
task configuration intoscopedSettings
and define values likesources in playEbeanQuery := resourceDirectory.value
andtarget in playEbeanQuery := crossTarget.value / "ebean-query" / Defaults.nameForSrc(configuration.value.name)
. - Bind these settings using
inConfig(Compile)(scopedSettings) ++ inConfig(Test) ++ ...
so it will pick up the different directory values when compiling and testing (see raw and configured settings).
- Here is the best practices guide which suggests reusing keys: http://www.scala-sbt.org/0.13/docs/Plugins-Best-Practices.html#Reuse+existing+keys.
- Here is the list of keys: http://www.scala-sbt.org/0.13/sxr/sbt/Keys.scala.html.
- You can see how Play binds keys too, so you can reference directories like the
app
directory without hardcoding them: https://github.com/playframework/playframework/blob/master/framework/src/sbt-plugin/src/main/scala/play/sbt/PlayLayoutPlugin.scala.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I'm not sure about that.
Twirl and Query bean generation are two separate things.
When you're writing a twirl template, you only have one canonical source file: the .scala.html that the user actively works with to fulfil their tasks. This is compiled in to a mostly unreadable scala file that is used internally by Play that is then compiled down to an actual class file by the compiler.
In general the user has no interest in the intermediate Scala file so it should be not managed by source control and should be stored in the target folder as it is fine to discard and rebuild the contents of this folder.
This use case is different : if you peruse the example project, you will see that they have simply checked the generated source code into source control and more importantly they have modified the query beans after the initial generation to add additional functionality to them.
If we place it within the managed source directory like this (by default), we are essentially limiting users to only using the autogenerated functionality which is the opposite intent of what I was trying to achieve (based upon the Ebean examples).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewl102 can you share a link to the example project you're talking about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/ebean-orm/avaje-ebeanorm-examples .
The a-basic directory contains an example of usage of query beans.
This is only an example obviously, however it seems quite useful to modify the auto generated finder classes to provide richer functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, sounds like the thing to do then is put the query beans into a managed source directory as Rich suggested. That's how SBT plugins would typically handle this from what I've seen.
We could potentially invoke the code generator twice. Once to generate the never-modified query beans into the managed source directory and once to generate the usually modified Finders into the regular source directory. For simplicity though maybe it's easiest to just generate the query beans and forget about the Finders. The Finders seem rather simple compared to the query beans, so most of the value in the code generation is in generating the query beans, so that seems good enough for a v1 of this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be useful, after reading the previous comments I thought perhaps the way to go was finders -> source control and regular beans -> managed sources.
Ideally this could be done in the same step however it probably requires modification of the existing generation code.
If I ran it twice I'd also have to purge the generated query classes.
I think it's probably worth either making a modification to the other project or just ignoring it as you suggested. The user can always manually copy it to their source control although they'd have to disable the generation afterwards possibly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for simplicity we just don't give the user the ability to generate Finders for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've spent some time on this and hit some severe problems.
The query generation code requires the model classes already be compiled. By moving it to a generated source, we now have the condition where we need the sources for the models to be compiled before we trigger the query generation phase.
Even if I manage to bind this at the correct point after the model compilation has taken, it now sets up a scenario where in the person has written code that is dependent on the generated query beans, which will now not compile, preventing the original compilation of the model classes from taking place either if they do a clean compile.
It might be possible to solve this but it's way beyond the difficulty I had originally intended.
I would somehow have to trigger only the compilation of the model classes, without any dependent code, for the purposes of completing this phase with the proposed setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being MIA and not reviewing over the holidays.
That's a good point you bring up. Forget that idea then. Can you add a comment to the code explaining why we're not using managed sources? Don't want someone else trying to go down that path in the future without knowing what they're getting into.
I think if we're not going to use managed sources then this is probably better as a task so that you have to type the task name on the console to trigger it. That way we're not automatically overwriting code you have checked in. Hopefully that will be a much easier change to make.
playEbeanQueryEnhance := false, | ||
playEbeanQueryDestDirectory := "app", | ||
playEbeanQueryResourceDirectory := "conf", | ||
playEbeanQueryModelsPackage := "models", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think users should be able to configure this or the generator must respect models packages even if it is not using the default models
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be configurable.
If the user specifies playEbeanQueryModelsPackage := "notmodels" in their build.sbt it will override the default models definition unless I'm not understanding your comment correctly.
@andrewl102 can you squash the commits? |
"com.typesafe" % "config" % "1.3.0" | ||
) | ||
|
||
def avajeEbeanormAgent = "org.avaje.ebeanorm" % "avaje-ebeanorm-agent" % "4.7.1" | ||
def avajeEbeanormQueryAgent = "org.avaje.ebeanorm" % "avaje-ebeanorm-typequery-agent" % "1.5.1" | ||
def avajeGenerator = "org.avaje.ebeanorm" % "avaje-ebeanorm-typequery-generator" % "1.5.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the latest is 1.5.3
will this make it into play 2.5? |
@oexza it don't need to. :-) The main benefit to have play-ebean as a separated project is that we can release new versions of it independent of Play lifecycle. By independent I mean that we can later release a new version of play-ebean with query bean support without releasing a new version of Play. |
For java what are the flags |
ping! @andrewl102 @benmccann 😃 is this PR still alive? |
hello guys, still gonna nag you on this @andrewl102 @benmccann @marcospereira where are we at on this Type safety goodness? why is this stalled? is it abandoned? |
As a note the Ebean maven artifacts above have since be renamed and updated so:
Should ultimately be moved over to: The "finder-generator" (renamed from avaje-ebeanorm-typequery-generator) ... generates finders but it also generates query beans by reading the entity bean information from compiled classes. I believe this is used because we can't use Java annotation processing. Ebean's "querybean-generator" artifact is the Java annotation processor that generates query beans (so as I understand it we can't use it at the moment with sbt). Cheers, Rob. |
@oexza we stopped working on this due to slight changes in tech stack at Connectifier after being acquired. if you'd like to take over I will be happy to review, but we don't have time to implement ourselves |
I'd love to see this merged. Or is there any way to use query beans without modifications to play-ebeans? I have managed to get querybeans generation working by just adding Is there a simple way to enable enhancements? |
@aviau Hi, After adding library dependency I am getting compiler warnings saying, " What other dependency you have added to make it work. I am using Play Framework 2.5.14. Part of my build.sbt contains,
|
Hi |
Hey @AlekseyMko, this needs to be revamped given the last updates and changes made in play-ebean. Right now it is not even mergeable since there are conflicts with the current code. I'm not sure if @andrewl102 still have time to move this forward, but if not, someone can cherry-pick his commits here, solve the conflicts and submit a new pull request. What do you think? |
Hi @marcospereira. I updated code from this pull request and fixed to make it work. But I did it only for 3.1.1 version of play-ebean plugin. 4.0.0 Uses newer version of ebean and code requires much more changes. |
I worked a bit with my updated plugin and it couses reloading of application after each request as ebean Generator does not care about changes in models and just regenerates finders and query beans every time. This forces sbt to reload application and makes development process unacceptable. |
Any news on that? |
resolve #61