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 synthetic JavaBean getters to AutoValue classes #3188

Merged
merged 13 commits into from Dec 13, 2016

Conversation

Projects
None yet
2 participants
@joschi
Contributor

joschi commented Dec 12, 2016

Description

This PR adds an AutoValue extension based on #3127 (comment) which generates synthetic getters following the JavaBean conventions.

It also adds the new @WithBeanGetter annotation to the relevant AutoValue classes.

Fixes #3127

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code follows the code style of this project.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

joschi added some commits Dec 12, 2016

@joschi joschi added this to the 2.2.0 milestone Dec 12, 2016

@joschi joschi requested a review from bernd Dec 12, 2016

final Map<String, ExecutableElement> properties = context.properties();
for (Map.Entry<String, ExecutableElement> entry : properties.entrySet()) {
typeSpecBuilder.addMethod(generateGetterMethod(entry.getKey(), entry.getValue()));

This comment has been minimized.

@bernd

bernd Dec 13, 2016

Member

We have to check if there are get*/is* methods on the class already so we don't override them.

This comment has been minimized.

@joschi

joschi Dec 13, 2016

Contributor

I thought about that but decided against the check because we explicitly add the @WithBeanGetter annotation to classes. If these classes already contain getters, we don't add the annotation.

* Creates Java Bean getter methods for each property.
*/
public class WithBeanGetterExtension extends AutoValueExtension {
private static final ClassName ANNOTATION_JSON_PROPERTY = ClassName.get(JsonProperty.class);

This comment has been minimized.

@bernd

bernd Dec 13, 2016

Member

I have been using the ClassName.get("com.fasterxml.jackson.annotation", "JsonIgnore"); version in the original code so we don't have to depend on jackson. If we want to extract this extension into a separate project later - which I think is a good idea because this might be useful to others as well - we should probably use that code again.

This comment has been minimized.

@joschi

joschi Dec 13, 2016

Contributor

The fact that we have to process the Jackson annotations (@JsonProperty and @JsonIgnore) should be obvious. I think it's safe to depend on Jackson and use the class objects directly in this case.

@@ -31,6 +32,7 @@
import org.mongojack.ObjectId;
@AutoValue
@WithBeanGetter

This comment has been minimized.

@bernd

bernd Dec 13, 2016

Member

Why do we want to add the annotation to every AutoValue class we have? This only makes sense for those that have validation annotations, no?

This comment has been minimized.

@joschi

joschi Dec 13, 2016

Contributor

Yes, but I didn't want to check each class individually and used some sed magic to replace @AutoValue with @AutoValue\n@WithBeanGetters (minus those classes which already contained getters). 😉

This comment has been minimized.

@bernd

bernd Dec 13, 2016

Member

Okay, fair enough. 👍

final List<String> methodNames = Arrays.stream(bean.getClass().getMethods())
.map(Method::getName)
.collect(Collectors.toList());
assertThat(methodNames)

This comment has been minimized.

@bernd

bernd Dec 13, 2016

Member

Please add a test that checks if the annotations are pulled over to the new method. Thanks!

This comment has been minimized.

@joschi

joschi Dec 13, 2016

Contributor

@bernd bernd self-assigned this Dec 13, 2016

joschi added some commits Dec 13, 2016

@bernd

This comment has been minimized.

Member

bernd commented Dec 13, 2016

Looks good to me so far. Let me know once it's ready for a final review.

@joschi joschi referenced this pull request Dec 13, 2016

Closed

Support for multiple retention strategies #2880

23 of 27 tasks complete
@@ -416,6 +417,17 @@
<scope>provided</scope>
</dependency>
<dependency>
<groupId>com.gabrielittner.auto.value</groupId>
<artifactId>auto-value-extension-util</artifactId>
<version>${auto-value-extension-util.version}</version>

This comment has been minimized.

@bernd

bernd Dec 13, 2016

Member

Shouldn't this be marked as provided to avoid pulling it into the regular dependencies of the server? Or is this implicitly provided because the auto-value-javabean itself is in scope provided?

This comment has been minimized.

@joschi

joschi Dec 13, 2016

Contributor

The latter, it's a compile dependency for auto-value-javabean but will not be pulled in transitively in graylog2-server, because auto-value-javabean is pulled in as provided there.

This comment has been minimized.

@bernd
@@ -115,6 +116,7 @@ private IndexSetConfig findDefaultIndexSet() {
@JsonAutoDetect
@AutoValue
@WithBeanGetter

This comment has been minimized.

@bernd

bernd Dec 13, 2016

Member

Nitpick: indentation is broken (for several classes)

This comment has been minimized.

@joschi

joschi Dec 13, 2016

Contributor

👍

* You should have received a copy of the GNU General Public License
* along with Graylog. If not, see <http://www.gnu.org/licenses/>.
*/
package org.graylog2.autovalue;

This comment has been minimized.

@bernd

bernd Dec 13, 2016

Member

I guess this file should be in the auto-value-javabean maven module instead of the the graylog2-server module?

This comment has been minimized.

@joschi

joschi Dec 13, 2016

Contributor

No, unfortunately that's not possible as the AutoValue extension isn't available at compile time. We'd have to move this test into a separate project or fall back to "runtime" compilation in our tests (similar to https://github.com/gabrielittner/auto-value-with/tree/master/auto-value-with/src/test/java/com/gabrielittner/auto/value/with).

The given solution seemed the most simple one to achieve our goal (although not the cleanest one).

This comment has been minimized.

@bernd

bernd Dec 13, 2016

Member

Bummer! Thanks!

@@ -29,6 +30,7 @@
import java.util.Map;
@AutoValue
@WithBeanGetter

This comment has been minimized.

@bernd

bernd Dec 13, 2016

Member

This class, KeywordRange and RelativeRange all implement their own #getFrom() and #getTo() due to the abstract methods in TimeRange.

Maybe we should remove the annotation from those to avoid confusion in the future if any of those hand written implementations should change.

This comment has been minimized.

@joschi

joschi Dec 13, 2016

Contributor

👍

joschi added some commits Dec 13, 2016

Remove stray newlines
[ci skip]
@bernd

bernd approved these changes Dec 13, 2016

@bernd bernd merged commit 5c8db63 into master Dec 13, 2016

2 checks passed

ci-web-linter Jenkins build graylog-pr-linter-check 1140 has succeeded
Details
licence/cla Contributor License Agreement is signed.
Details

@bernd bernd deleted the issue-3127 branch Dec 13, 2016

@bernd bernd removed the ready-for-review label Dec 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment