CCDM: check for null in returned Java Bean from Connect service methods

[platosha]

Connected to #6717

---

This change is 

[qtdzz]

Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @platosha)

---

flow-server/src/main/java/com/vaadin/flow/server/connect/ExplicitNullableTypeChecker.java, line 117 at r1 (raw file):

            for (PropertyDescriptor propertyDescriptor : Introspector
                    .getBeanInfo(clazz).getPropertyDescriptors()) {

The generator ignores static, transient, and @JsonIgnore, so we should also ignore them here because we are not interested in checking them.
Also the @Nullable property should be excluded from the check. I quickly added the following code before the loop:

            for (Field field : clazz.getFields()) {
                boolean shouldIgnore = Modifier
                        .isStatic(field.getModifiers())
                        || Modifier.isTransient(field.getModifiers())
                        || field.isAnnotationPresent(JsonIgnore.class)
                        || field.isAnnotationPresent(Nullable.class);
                if (!shouldIgnore) {
                    validProperties.add(field.getName());
                }
            }

---

flow-server/src/main/java/com/vaadin/flow/server/connect/ExplicitNullableTypeChecker.java, line 118 at r1 (raw file):

            for (PropertyDescriptor propertyDescriptor : Introspector
                    .getBeanInfo(clazz).getPropertyDescriptors()) {
                Method readMethod = propertyDescriptor.getReadMethod();

The readMethod can be null if there is no getter. Should we check for the raw value in that case?

---

flow-server/src/main/java/com/vaadin/flow/server/connect/ExplicitNullableTypeChecker.java, line 130 at r1 (raw file):

                }
            }
        } catch (IntrospectionException e) {

These exceptions can be combined in one catch clause

         } catch (IntrospectionException | InvocationTargetException
                | IllegalAccessException e) {
            return e.toString();
         }

but you also might need to log the exception to satisfy the Sonarqube

[platosha]

Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @qtdzz and @vaadin-bot)

---

flow-server/src/main/java/com/vaadin/flow/server/connect/ExplicitNullableTypeChecker.java, line 117 at r1 (raw file):

Previously, qtdzz (Tien Nguyen) wrote…

            for (PropertyDescriptor propertyDescriptor : Introspector
                    .getBeanInfo(clazz).getPropertyDescriptors()) {

The generator ignores static, transient, and @JsonIgnore, so we should also ignore them here because we are not interested in checking them.
Also the @Nullable property should be excluded from the check. I quickly added the following code before the loop:

            for (Field field : clazz.getFields()) {
                boolean shouldIgnore = Modifier
                        .isStatic(field.getModifiers())
                        || Modifier.isTransient(field.getModifiers())
                        || field.isAnnotationPresent(JsonIgnore.class)
                        || field.isAnnotationPresent(Nullable.class);
                if (!shouldIgnore) {
                    validProperties.add(field.getName());
                }
            }

Done.

---

flow-server/src/main/java/com/vaadin/flow/server/connect/ExplicitNullableTypeChecker.java, line 118 at r1 (raw file):

The readMethod can be null if there is no getter. Should we check for the raw value in that case?

OK per spec it must contain a getter method to enable introspection.

---

flow-server/src/main/java/com/vaadin/flow/server/connect/ExplicitNullableTypeChecker.java, line 130 at r1 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

Either log or rethrow this exception.

Done.

---

flow-server/src/main/java/com/vaadin/flow/server/connect/ExplicitNullableTypeChecker.java, line 130 at r1 (raw file):

Previously, qtdzz (Tien Nguyen) wrote…

These exceptions can be combined in one catch clause

         } catch (IntrospectionException | InvocationTargetException
                | IllegalAccessException e) {
            return e.toString();
         }

but you also might need to log the exception to satisfy the Sonarqube

Done.

---

flow-server/src/main/java/com/vaadin/flow/server/connect/ExplicitNullableTypeChecker.java, line 132 at r1 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

Either log or rethrow this exception.
Combine this catch with the one at line 130, which has the same body.

Done.

---

flow-server/src/main/java/com/vaadin/flow/server/connect/ExplicitNullableTypeChecker.java, line 134 at r1 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

Either log or rethrow this exception.
Combine this catch with the one at line 130, which has the same body.

Done.

[qtdzz]

Reviewed 2 of 2 files at r2.
Reviewable status: 10 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @platosha, @qtdzz, and @vaadin-bot)

---

flow-server/src/main/java/com/vaadin/flow/server/connect/ExplicitNullableTypeChecker.java, line 178 at r2 (raw file):

Quoted 16 lines of code…

                Class<?> declaringClass = readMethod.getDeclaringClass();
                final Set<String> declaredFieldNames = Arrays
                        .stream(declaringClass.getDeclaredFields())
                        .map((Field field) -> field.getName())
                        .collect(Collectors.toSet());
                if (!declaredFieldNames.contains(name)) {
                    continue;
                }

                Field field = declaringClass.getDeclaredField(name);
                if (Modifier.isStatic(field.getModifiers())
                        || Modifier.isTransient(field.getModifiers())
                        || field.isAnnotationPresent(JsonIgnore.class)
                        || field.isAnnotationPresent(Nullable.class)) {
                    continue;
                }

We can extract this out as a method and combine it with the above continue statement.
Something like this:

    private boolean shouldSkip(Method readMethod, String fieldName)
            throws NoSuchFieldException {

        Class<?> declaringClass = readMethod.getDeclaringClass();
        final Set<String> declaredFieldNames = Arrays
                .stream(declaringClass.getDeclaredFields())
                .map(Field::getName)
                .collect(Collectors.toSet());
        if (!declaredFieldNames.contains(fieldName)) {
            return true;
        }

        Field field = declaringClass.getDeclaredField(fieldName);
        return Modifier.isStatic(field.getModifiers()) ||
                Modifier.isTransient(field.getModifiers()) ||
                field.isAnnotationPresent(JsonIgnore.class) ||
                field.isAnnotationPresent(Nullable.class);
    }

---

flow-server/src/main/java/com/vaadin/flow/server/connect/ExplicitNullableTypeChecker.java, line 193 at r2 (raw file):

        } catch (IntrospectionException | InvocationTargetException
                | IllegalAccessException | NoSuchFieldException e) {
            getLogger().error(e.getMessage());

I think we should return e.getMessage() and log error(e)

[vaadin-bot]

Remove this unused method parameter "clazz".

[vaadin-bot]

SonarQube analysis reported 1 issue

• 1 major

Watch the comments in this conversation to review them.

[qtdzz]

ps: the major sonarqube issue will be fixed in the incoming follow up PR

Reviewed 2 of 2 files at r3.
Reviewable status: 8 unresolved discussions, 1 of 1 LGTMs obtained (waiting on @platosha, @qtdzz, and @vaadin-bot)

[qtdzz]

Dismissed @vaadin-bot from 4 discussions.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained