-
-
Notifications
You must be signed in to change notification settings - Fork 367
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
Automatic table configuration via Annotation Processing #45
base: main
Are you sure you want to change the base?
Conversation
Add preliminary code that will eventually replace table config files with a class containing inline code to build the data structures that is generated automatically when the annotated client code is compiled.
Since the javac compiler does multiple passes over the source code and calls process once each pass, it is better to create a separate file for each class.
This reverts commit d2fd0da.
Hi guys, thanks @NCrouther for this. I am actually working on an OrmLite android Plugin. The plugin is ready but uses the ORMLite Config Util class to generate the ormlite configuration file. And this approach has several limitations that would all be removed by using an annotation processor. The plugin works fine, but with several limitations. Nevertheless, it is production ready and we already use it at Groupon internally. We got large speed improvements but it twists a bit our build cycle and it creates some problem at the SCM tree level (basically the ormlite config file, which is now generated has still to be part of the tree). An annotation processor would get rid of those twists and SCM troubles. The annotation processor is really the way to go but I didn't have time to look into it. I am glad someone could find some time and spent some effort on this. I plan to review the PR this week, if you are fine with that @j256 . Also, @j256 , I think this all should be merged in ORMLite : both the annotation processor and the android gradle plugin. |
* | ||
* @author nathancrouther | ||
*/ | ||
@Retention(RetentionPolicy.SOURCE) |
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.
Good Javadoc but please add a <pre>
block with code 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.
Also, here I am not sure I would use annotations more than a programmatic API to declare classes. Are both ways still supported ?
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.
Also, here I am not sure I would use annotations more than a programmatic API to declare classes. Are both ways still supported ?
The annotation processor requires all of the table classes for the database to be declared in an annotation so that it can generate the code to cache the table configurations with the DaoManager. I wanted to avoid the user needing to list the tables twice (in their onCreate [as before] and in the newly added Database annotation), so I added an autogenerated function that can be used to help implement onCreate so that the list of tables only appears once.
Old code will continue to work unchanged, but it won't get the benefit of increased performance that the annotation processors provide. The annotation processor will raise a warning in this case to encourage the user to add the annotation.
The alternative would be to not generate a class that collects all of the individual TableConfigs and just have the user register each of them with the DaoManager in their constructor body. I didn't like this approach since they would need to call a generated class function for each table in addition to the existing create call for each table. Since from an IDE point of view the generated classes aren't connected to the DatabaseTable classes, it would add burden to refactorings (e.g. renaming table classes).
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 added some code examples to flesh out the javadoc: 275a47a.
Generally speaking, a gradle / android example is missing. It should use |
@@ -0,0 +1,812 @@ | |||
package com.j256.ormlite.android.annotations; |
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.
We should rename that package to annotation_processor or processor. The annotations package should only contains the ormlite annotations.
This would avoid scoping problems where the annotations are still needed at runtime but the annotation processor should be on the provided / apt scope and not included in the final apk.
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 renamed the package for that class. bfd65be
Is there a way to drop that package at runtime without moving it into a separate JAR? I was hoping to avoid creating another project for the annoation processor since it shouldn't take too much space at runtime.
writer.write("}\n"); | ||
} | ||
|
||
private static boolean writeSetterIfNotDefault(Annotation annotation, |
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 boolean returned here is rarely used. Same for writeSetterIfNotEqual
below. Would there be a way to split this method in 2 methods : one that test things and return a boolean and one that actually writes.
It looks like the API of writeSetterIfNotDefault
(both) is not clear because it seems to violate the command query separation principle.
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.
Yeah, it is not ideal and probably a little confusing. I am having trouble coming up with a cleaner design. I only want to generate lines of code for annotation properties that the user set to minimize the amount of code generated, so I reflectively get the default and the actual value, compare them, and if they don't match, generate a setter call. There are two deprecated properties in ForeignCollectionField that have replacements. The code first checks if the user has set the new names and if not, falls back to the deprecated names.
Since most of the code involved is around extracting the default and actual values (it is pretty verbose because properties of type Class<?> require special case handling), refactoring it into separate query and command functions would add a lot of duplicated code.
I added javadoc to the method explaining the return value: 6c14b03.
I have a few comments, but I think this PR is really great ! I hope it can be made stable and merged soon to ORMLite. |
Add better comments justifying why each exception is or is not fatal and make sure every unexpected exception is logged.
Change the package declaration in the unit test input resource files to be under the processor package
Thanks for all of the detailed feedback. I knocked out all of the easy stuff today, I should have time to tackle the rest of it later this week. A couple of open questions: How should we handle the new dependency on a java code generation library (Velocity or JavaPoet)? I was hesitant to use a library since currently the Android JAR has no runtime dependencies. I know I have used ORMLite by just adding the JAR to an eclipse project, and I would guess other users do this as well; I didn't want to add too much complexity for those users. My research on the issue pointed me to the maven shade plugin which looks like will bundle the dependency into the single JAR. Is this a good way to go? Is there anybody familiar with it that can point me in the right direction? Should the annotation processing code be in a separate JAR? The gradle apt plugin looks like it is built around having a JAR with just the code for annotation processing so it can load it during compile but not add it to the output JAR. I can see the benefit, but since the annotation processing code is pretty small for now, I figured it would be simpler to keep it all one project. If we decide to split it, is that something I should do (i.e. I would create a project for it and the j256 repo would fork it), or should this repo split it out? My gut feel would be to keep it together for now and if the annotation processing grows (such as if we do more code generation to eliminate reflection when populating query result objects), @j256 would create the new project and migrate the annotation processing code there. Thoughts? |
I went ahead and added JavaPoet and used the maven shade plugin to eliminate the added dependency. It does bump the size of the annotation processing code from ~10KB to ~70KB which adds some more motivation to split the annotation processing into a separate JAR that can be discarded at runtime TODOs:
Open Questions:
|
I looked at how Dagger handles creating two JARs and they use a maven multipke module project, which seems like the best way to go. There are a couple of areas that I could use some guidance:
|
Hi @NCrouther , you are right, for the jar management. It's better to use modules. I would suggest the following : (but in a different PR as it involves changing orm lite android's pom)
But this might be a little long and should maybe be addressed in a different PR. Already, this, which is quite a change, and has to be merged. For Javadoc, I don't know. @j256 @kpgalligan What do you think of this ? |
Thanks for the confirmation. I solved the javadoc issue by using the aggregate-jar target for the javadoc plugin in the parent pom. I have the project split into the modules you describe and everything seems to be working properly. However, there are still sections of the POM (such as the sourceforge and sonatype profile sections) that I don't understand, so they will definitely need heavy review. Should I commit the changed pom (along with moving all source files to appropriate folders) to this PR or wait and create a new PR? |
I don't know what the plan of @j256 is. The easiest would be a PR against your branch, not an additional commit and a link to follow up on the PR (which is gonna be in your repo). Good work ! Sonatype and sourceforge stuff should not be modified. But they should bubble up to root level. |
Thanks for the advice. I'm really new to Git/GitHub, so I appreciate the hand-holding, I'm learning a lot. I followed your recommendation and put the initial version of the module-split pom into a separate branch/PR in my repo: NCrouther#2. Based on the JARs created, functionally everything seems good, but I am sure the POMs can use some further refactoring. |
I only saw this PR now, but it's really similar to a project I started last Friday: https://github.com/koesie10/ormlite-processor. It's basically the same, but without changing the project's code and it has some limitations. |
Getting a build error from the JavaPoet. I think the build needs to be 1.7, but changing to that throws a different issue. JavaPoet related issue: [ERROR] /Users/kgalligan/Dropbox (Touch Lab)/devel_git/ormliterefactor/ormlite-android/src/main/java/com/j256/ormlite/android/processor/OrmLiteAnnotationProcessor.java:[453,18] cannot access java.nio.file.Path Also, how do you add to a gradle project? Example? Working on this today. |
Didn't mention @NCrouther directly in last message. Not sure if you'll see it... |
Sorry for the delay, I was away from email over the holiday weekend. I didn't realize that JavaPoet would increase the required JRE/JDK version. Hopefully the impact will be small since I am splitting it into two separate JARs. The main JAR can retain 1.5 compatibility, and the annotation processor JAR can target the higher version that is necessary. I am doing some experiments on the branch with the pom changes: NCrouther#2 I incorporated the animal sniffer plugin (http://mojo.codehaus.org/animal-sniffer/) to verify that the code is not using any newer APIs than are present in the declared java version. It seems like everything is fine on 1.6. This change should fix the java.nio.file.Path not found problem, the real issue will be figuring out the other error you ran into when changing that. Let me know what happens when you try to build the latest from that branch. |
To add to gradle (for the branch with the annotation processing split into a separate JAR):
Once this version is deployed to maven central, it will be:
|
You should add a project type dependency it will handle both cases. Let me know if I can review the code. S.
|
This pull request adds an annotation processor that examines annotations at compile time and generates a function that adds cached config information for all tables to the DAO Manager. This is intended to replace textual config files, with the advantage that it is generated automatically every build. All reflection is done at compile time, so there is no runtime cost. Simple inputs and outputs are included as automated unit tests.