You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/12/28 15:47:18 UTC
[GitHub] [beam] krakowski opened a new pull request #13622: [BEAM-11530] Annotated setter parameters handled wrong in schema creation
krakowski opened a new pull request #13622:
URL: https://github.com/apache/beam/pull/13622
I'm facing an exception telling me that my JavaBean contained a setter for a field that had a mismatching nullable attribute, although both getter and setter (parameters) are annotated with `@Nullable` (`org.apache.avro.reflect.Nullable`).
```java
@DefaultSchema(JavaBeanSchema.class)
public final class SampleBean {
@Nullable
private Float value;
public @Nullable Float getValue() {
return value;
}
public void setValue(@Nullable Float value) {
this.value = value;
}
}
```
The code responsible for determining if a setter's parameter is nullable looks wrong to me, since it does check if the type (class) itself is annotated with `@Nullable` instead of checking the actual parameter's annotations.
This PR removes the intermediate call to `getAnnotatedType()` so that the annotations placed in front of the parameter are returned instead of those placed on top of the annotated type's class.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [x] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [x] Update `CHANGES.md` with noteworthy changes.
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam
.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website | Whitespace | Typescript
--- | --- | --- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski commented on pull request #13622: [BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752471261
I removed the custom annotation and replaced it with `org.apache.avro.reflect.Nullable`. I also refactored the methods responsible for checking if a `@Nullable` annotation is present (e.g [here](https://github.com/apache/beam/pull/13622/files#diff-beba37fb866fda6032626b6e228d47968f6ff0fee229246061d1fb96b04355cbR192-R198)).
I didn't really know where to put the comment, so I placed it [on top of the test method](https://github.com/apache/beam/pull/13622/files#diff-404854b8f39406bdf5683ce4ae69f4ca4d055a40511e5c8208aeee9d4fd6ee0aR222-R234) as a javadoc.
It looks like I ran into some kind of race condition by force pushing twice in a row since the Dataflow check is failing because of a locking problem.
> Cannot lock Java compile cache (/home/jenkins/jenkins-slave/workspace/beam_PreCommit_Java_Examples_Dataflow_Java11_Commit/src/.gradle/6.7.1/javaCompile) as it has already been locked by this process.
Is it possible to rerun this check?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski commented on pull request #13622: [DO NOT MERGE][BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752154819
Just to be sure I created a `@Nullable` annotation inside `org.apache.beam.sdk.schemas.utils` (which should be on the classpath) and replaced `org.apache.avro.reflect.Nullable` with `org.apache.beam.sdk.schemas.utils.Nullable`.
```java
package org.apache.beam.sdk.schemas.utils;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
@Retention(RetentionPolicy.RUNTIME)
public @interface Nullable {}
```
Unfortunately the test is still failing with the same message.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #13622: [BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-751907821
Thanks for the contribution @krakowski! It looks like this change actually breaks our existing test though: https://ci-beam.apache.org/job/beam_PreCommit_Java_Commit/15285/testReport/junit/org.apache.beam.sdk.schemas/JavaBeanSchemaTest/testNullableToRow/
Maybe if you add a test for your use case to `JavaBeanSchemaTest` we can find a fix that works for both?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #13622: [BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752212662
> or should I revert to using org.apache.avro.reflect.Nullable and remove the custom annotation?
Let's just use the avro Nullable annotation
> Found the answer at https://stackoverflow.com/a/37587590/5896429
Nice! Thank you. Could you add a comment linking to that with a brief explanation?
It looks like CI is failing because of the spotless format check - you can autoformat with `./gradlew spotlessApply`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski commented on pull request #13622: [BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-751759690
R: @TheNeuralBit
R: @kennknowles
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski commented on pull request #13622: [BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752734701
I rebased on top of `master` an reran `spotlessApply` so everything should be working now. After the checks have passed, this is ready to be merged from my side :+1:
Thanks for reviewing this and giving advice! :slightly_smiling_face:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski commented on pull request #13622: [DO NOT MERGE][BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752174294
I replaced Avro's `Nullable` with a custom one and pushed my changes. Is it ok to place the custom `Nullable` annotation inside `org.apache.beam.sdk.schemas.utils` or should I revert to using `org.apache.avro.reflect.Nullable` and remove the custom annotation?
> I'd like to understand why it's necessary.
I am trying to figure that out too :smile:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski commented on pull request #13622: [BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752722213
Thanks!
I applied the changes through the GitHub UI which lead to two additional commits. Should i squash all into one commit and force push again?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski edited a comment on pull request #13622: [DO NOT MERGE][BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski edited a comment on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752154819
Just to be sure I created a `@Nullable` annotation inside `org.apache.beam.sdk.schemas.utils` (which should be on the classpath) and replaced `org.apache.avro.reflect.Nullable` with `org.apache.beam.sdk.schemas.utils.Nullable`.
```java
package org.apache.beam.sdk.schemas.utils;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
@Retention(RetentionPolicy.RUNTIME)
public @interface Nullable {}
```
Unfortunately the test is still failing with the same message because `method.getParameters()[0].getAnnotatedType().getAnnotations()` is empty.
The funny thing is, I can access this annotation using `method.getParameters()[0].getAnnotations()` :thinking:
[](https://i.imgur.com/N0RTnu0.png)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #13622: [DO NOT MERGE][BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752146612
Ah thanks for the investigation. When looking into BEAM-10632 I learned that the Java spec requires the runtime to ignore an annotation whose type isn't on the classpath:
> When an annotation is put on a member but the annotation type is missing on the class path, the specification requires the runtime to ignore this annotation, i.e. to act as if the annotation is not present.
Is it possible `org.apache.avro.reflect.Nullable` isn't on the classpath?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #13622: [BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752687795
Run Java_Examples_Dataflow_Java11 PreCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski commented on pull request #13622: [DO NOT MERGE][BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752143462
Using `org.apache.avro.reflect.Nullable` instead of `org.checkerframework.checker.nullness.qual.Nullable` seems to be it.
I reverted my changes and added a failing unit test showing my issue. I can't explain why this is happening because both annotations have `RetentionPolicy.RUNTIME` and should therefore be visible during runtime.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit merged pull request #13622: [BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
TheNeuralBit merged pull request #13622:
URL: https://github.com/apache/beam/pull/13622
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski edited a comment on pull request #13622: [DO NOT MERGE][BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski edited a comment on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752154819
Just to be sure I created a `@Nullable` annotation inside `org.apache.beam.sdk.schemas.utils` (which should be on the classpath) and replaced `org.apache.avro.reflect.Nullable` with `org.apache.beam.sdk.schemas.utils.Nullable`.
```java
package org.apache.beam.sdk.schemas.utils;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
@Retention(RetentionPolicy.RUNTIME)
public @interface Nullable {}
```
Unfortunately the test is still failing with the same message because `method.getParameters()[0].getAnnotatedType().getAnnotations()` is empty.
The funny thing is, I can access this annotation using `method.getParameters()[0].getAnnotations()` :thinking:
[](https://i.imgur.com/N0RTnu0.png)
Maybe a solution would be to check both paths for the existence of `@Nullable`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #13622: [DO NOT MERGE][BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752171809
Very strange indeed. I found that for the Avro Nullable case `method.getParameterAnnotations()[0]` will also work (maybe this is just a shorthand for `method.getParameters()[0].getAnnotations()`. It seems like a reasonable solution to check both paths, but I'd like to understand why it's necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski commented on pull request #13622: [DO NOT MERGE][BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752178161
Found the answer at https://stackoverflow.com/a/37587590/5896429
Apparently the annotation has to be declared with `ElementType.TYPE_USE` in order to be accessible through `getAnnotatedParameterTypes()`. Learned something new.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on pull request #13622: [BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752726807
> I applied the changes through the GitHub UI which lead to two additional commits. Should i squash all into one commit and force push again?
That's ok, I can squash it when I merge.
It looks like my suggestions broke spotless though :facepalm:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski edited a comment on pull request #13622: [DO NOT MERGE][BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski edited a comment on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752154819
Just to be sure I created a `@Nullable` annotation inside `org.apache.beam.sdk.schemas.utils` (which should be on the classpath) and replaced `org.apache.avro.reflect.Nullable` with `org.apache.beam.sdk.schemas.utils.Nullable`.
```java
package org.apache.beam.sdk.schemas.utils;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
@Retention(RetentionPolicy.RUNTIME)
public @interface Nullable {}
```
Unfortunately the test is still failing with the same message because `method.getParameters()[0].getAnnotatedType().getAnnotations()` is empty.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] TheNeuralBit commented on a change in pull request #13622: [BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #13622:
URL: https://github.com/apache/beam/pull/13622#discussion_r550261069
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/FieldValueTypeInformation.java
##########
@@ -176,39 +177,38 @@ public static FieldValueTypeInformation forGetter(Method method) {
}
private static boolean hasNullableAnnotation(Field field) {
- for (Annotation annotation : field.getAnnotations()) {
- if (isNullableAnnotation(annotation)) {
- return true;
- }
- }
-
- for (Annotation annotation : field.getAnnotatedType().getAnnotations()) {
- if (isNullableAnnotation(annotation)) {
- return true;
- }
- }
+ Stream<Annotation> annotations =
+ Stream.concat(
+ Stream.of(field.getAnnotations()),
+ Stream.of(field.getAnnotatedType().getAnnotations()));
- return false;
+ return annotations.anyMatch(FieldValueTypeInformation::isNullableAnnotation);
}
/**
* If the method or its return type are annotated with any variant of Nullable, then the schema
* field is nullable.
*/
private static boolean hasNullableReturnType(Method method) {
- for (Annotation annotation : method.getAnnotations()) {
- if (isNullableAnnotation(annotation)) {
- return true;
- }
- }
+ Stream<Annotation> annotations =
+ Stream.concat(
+ Stream.of(method.getAnnotations()),
+ Stream.of(method.getAnnotatedReturnType().getAnnotations()));
- for (Annotation annotation : method.getAnnotatedReturnType().getAnnotations()) {
- if (isNullableAnnotation(annotation)) {
- return true;
- }
+ return annotations.anyMatch(FieldValueTypeInformation::isNullableAnnotation);
+ }
+
+ private static boolean hasSingleNullableParameter(Method method) {
+ if (method.getParameterCount() != 1) {
+ throw new RuntimeException("Setter methods should take a single argument.");
Review comment:
nit: Let's add the method name to this exception so it's more actionable
```suggestion
throw new RuntimeException("Setter methods should take a single argument " + method.getName());
```
##########
File path: sdks/java/core/src/test/java/org/apache/beam/sdk/schemas/JavaBeanSchemaTest.java
##########
@@ -216,6 +219,39 @@ public void testNullableFromRow() throws NoSuchSchemaException {
assertNull(bean.getStringBuilder());
}
+ /**
+ * [BEAM-11530] Java distinguishes between parameter annotations and type annotations. Therefore
+ * annotations declared without {@link java.lang.annotation.ElementType#TYPE_USE} can't be
+ * accessed through {@link Executable#getAnnotatedParameterTypes()}. Some {@code @Nullable}
+ * annotations like {@link org.apache.avro.reflect.Nullable} do not declare {@link
+ * java.lang.annotation.ElementType#TYPE_USE} which makes them parameter annotations once placed
+ * in front of a parameter.
+ *
+ * @see <a
+ * href="https://stackoverflow.com/a/37587590/5896429">https://stackoverflow.com/a/37587590/5896429</a>
+ * @see <a
+ * href="https://github.com/apache/beam/pull/13622">https://github.com/apache/beam/pull/13622</a>
+ */
Review comment:
Thank you this works :)
nit: we don't need the link to the PR, it can be found through `git blame` or the jira
```suggestion
*/
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski commented on pull request #13622: [BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski commented on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752116959
Hi @TheNeuralBit,
I tried reproducing the issue adding a unit test to `JavaBeanSchemaTest` but there it passes (with `getAnnotatedType()`), which I find pretty strange.
Because of this I created a separate repo for reproduction at https://github.com/krakowski/BEAM-11530.
Running `./gradlew run` within this projects results in the following stacktrace.
```
Exception in thread "main" java.lang.RuntimeException: JavaBean contained setter for field value that had a mismatching nullable attribute.
at org.apache.beam.sdk.schemas.utils.JavaBeanUtils.validateJavaBean(JavaBeanUtils.java:96)
at org.apache.beam.sdk.schemas.JavaBeanSchema.schemaFor(JavaBeanSchema.java:120)
at org.apache.beam.sdk.schemas.annotations.DefaultSchema$DefaultSchemaProvider.schemaFor(DefaultSchema.java:133)
...
```
The JavaBean used within this example is pretty simple and does contain only one field.
```java
package io.github.krakowski.beam;
import org.apache.avro.reflect.Nullable;
import org.apache.beam.sdk.schemas.JavaBeanSchema;
import org.apache.beam.sdk.schemas.annotations.DefaultSchema;
@DefaultSchema(JavaBeanSchema.class)
public final class SimpleBean {
@Nullable
private Float value;
public @Nullable Float getValue() {
return value;
}
public void setValue(@Nullable Float value) {
this.value = value;
}
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski edited a comment on pull request #13622: [DO NOT MERGE][BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski edited a comment on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752174294
I replaced Avro's `Nullable` with a custom one and pushed my changes. Is it ok to place the custom `Nullable` annotation inside `org.apache.beam.sdk.schemas.utils` or should I revert to using `org.apache.avro.reflect.Nullable` and remove the custom annotation?
> I'd like to understand why it's necessary.
I am trying to figure that out too :smile:
```java
Stream<Annotation> parameterAnnotations = Stream.concat(
Arrays.stream(method.getAnnotatedParameterTypes()[0].getAnnotations()),
Arrays.stream(method.getParameterAnnotations()[0])
);
```
At the moment this is my solution to the problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] krakowski edited a comment on pull request #13622: [DO NOT MERGE][BEAM-11530] Annotated setter parameters handled wrong in schema creation
Posted by GitBox <gi...@apache.org>.
krakowski edited a comment on pull request #13622:
URL: https://github.com/apache/beam/pull/13622#issuecomment-752154819
Just to be sure I created a `@Nullable` annotation inside `org.apache.beam.sdk.schemas.utils` (which should be on the classpath) and replaced `org.apache.avro.reflect.Nullable` with `org.apache.beam.sdk.schemas.utils.Nullable`.
```java
package org.apache.beam.sdk.schemas.utils;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
@Retention(RetentionPolicy.RUNTIME)
public @interface Nullable {}
```
Unfortunately the test is still failing with the same message because `method.getParameters()[0].getAnnotatedType().getAnnotations()` is empty.
The funny thing is, I can access this annotation using `method.getParameters()[0].getAnnotations()` :thinking:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org