ENG-5818vPersistOnly added optionals #1

samuellush · 2018-07-17T23:42:52Z

No description provided.

kwburgess · 2018-07-18T16:19:54Z

src/main/net/sf/persist/AnnotationTableMapping.java

@@ -115,6 +116,8 @@ public AnnotationTableMapping(final Class<?> objectClass) {
        notAutoGeneratedColumns = toArray(notAutoGeneratedColumnsList);
        autoGeneratedColumns = toArray(autoGeneratedColumnsList);

+        // TODO: implement optional


remove comment

kwburgess · 2018-07-18T16:21:46Z

src/main/net/sf/persist/AnnotationTableMapping.java

@@ -197,13 +200,27 @@ public String getTableName() {
    @Override
    public Method getGetterForColumn(final String columnName) {
        final String fieldName = columnsMap.get(columnName.toLowerCase(Locale.ENGLISH));
-        return gettersMap.get(fieldName);
+        if (fieldName == null) {


why do the if check log + catch a guaranteed NPE if the if check is true?
Wrap fieldName in Optional<>, do an "if present" check and throw an exception if it isn't. otherwise, return as normal. (e.g. no log needed, no try catch)

kwburgess · 2018-07-18T16:22:00Z

src/main/net/sf/persist/AnnotationTableMapping.java

    }

    @Override
    public Method getSetterForColumn(final String columnName) {
        final String fieldName = columnsMap.get(columnName.toLowerCase(Locale.ENGLISH));
-        return settersMap.get(fieldName);
+        if (fieldName == null) {


same as above

kwburgess · 2018-07-18T16:22:06Z

src/main/net/sf/persist/AnnotationTableMapping.java

+    @Override
+    public Class<?> getOptionalSubType(final String columnName) {
+        final String fieldName = columnsMap.get(columnName.toLowerCase(Locale.ENGLISH));
+        if (fieldName == null) {


same as above

kwburgess · 2018-07-18T16:50:59Z

src/main/net/sf/persist/AnnotationTableMapping.java

+            ENGINE_LOG.warn("Could not find field name corresponding to column name [" + columnName + "]");
+        }
+        try {
+            return gettersMap.get(fieldName);


discussed what you were trying to do here, wrap in an optional and throw a different persist exception for the inner map.
love the foresight for error cases!

kwburgess · 2018-07-18T17:22:01Z

src/main/net/sf/persist/NoTableMapping.java

@@ -114,7 +116,11 @@ public String getFieldNameForColumn(String columnName) {
    @Override
    public Method getSetterForColumn(String columnName) {
        String fieldName = getFieldNameForColumn(columnName);
-        return settersMap.get(fieldName);
+        try {
+            return settersMap.get(fieldName);


same as above

kwburgess · 2018-07-18T17:22:07Z

src/main/net/sf/persist/NoTableMapping.java

@@ -126,7 +132,22 @@ public Method getSetterForColumn(String columnName) {
    @Override
    public Method getGetterForColumn(String columnName) {
        String fieldName = getFieldNameForColumn(columnName);
-        return gettersMap.get(fieldName);
+        try {
+            return gettersMap.get(fieldName);


same as above

kwburgess · 2018-07-18T17:22:13Z

src/main/net/sf/persist/NoTableMapping.java

+    public Class<?> getOptionalSubType(String columnName) {
+        String fieldName = getFieldNameForColumn(columnName);
+        try {
+            return annotationsMap.get(fieldName).optionalSubType();


same as above

kwburgess · 2018-07-18T17:23:03Z

src/main/net/sf/persist/Persist.java

@@ -1031,8 +1045,8 @@ else if (type == 100) {
        }

        // for beans
+        // TODO optional


remove comment?

kwburgess · 2018-07-18T17:23:15Z

src/main/net/sf/persist/Persist.java

-                    final Object value = getValueFromResultSet(resultSet, i, type);
+                    final Class<?> optionalSubType = mapping.getOptionalSubType(columnName);
+
+                    // figure out these exception texts


remov e comment?

kwburgess · 2018-07-18T17:23:37Z

src/main/net/sf/persist/Persist.java

+                    // figure out these exception texts
+                    if ((type == Optional.class) && (optionalSubType == Void.class)) {
+                        throw new PersistException(
+                            "Column [" + columnName + "] was Optional but optionalSubType was Void.class");


what's wrong with the void class?

kwburgess · 2018-07-18T17:26:16Z

src/main/net/sf/persist/Persist.java

+                        throw new PersistException(
+                            "Column [" + columnName + "] was Optional but optionalSubType was Void.class");
+                    } else if ((type != Optional.class) && (optionalSubType != Void.class)) {
+                        PARAMETERS_LOG.error("Column type was not Optional but optionalSubType was not Void.class");


this is if someone populates "optionalSubType" but doesn't use optional as their main... okay.

kwburgess

👍 w/ comments

samuellush · 2018-07-18T21:36:47Z

This doesn't actually work as is. Persist reads in the column names from the table, finds corresponding annotations in the Value class, and maps the getters/setters, but you don't necessarily need every column to be represented in the Value class though with these changes you will get an exception

kwburgess · 2018-07-20T23:02:46Z

src/main/net/sf/persist/AnnotationTableMapping.java

+            }
+            return getterForFieldName.get();
+        }
+        return null;


LOG.warn - column exists in table that does not have a corresponding getter (or setter below) in the value class, ignoring column.

kwburgess · 2018-07-20T23:03:40Z

src/main/net/sf/persist/AnnotationTableMapping.java

+            }
+            return optSubTypeForFieldName.get();
+        }
+        return null;


if field name is not defined, we don't care, so return a null

kwburgess · 2018-07-20T23:03:46Z

src/main/net/sf/persist/AnnotationTableMapping.java

+            }
+            return serializationTypeForFieldName.get();
+        }
+        return null;


kwburgess · 2018-07-20T23:03:54Z

src/main/net/sf/persist/AnnotationTableMapping.java

+            }
+            return writerClassForFieldName.get();
+        }
+        return null;


kwburgess · 2018-07-20T23:04:04Z

src/main/net/sf/persist/AnnotationTableMapping.java

+                Optional.ofNullable(annotationsMap.get(fieldName.get()).writerClass());
+            if (!writerClassForFieldName.isPresent()) {
+                throw new NoSuchElementException(
+                    "Could not find writer class for columnn with field name [" + fieldName + "]");


columnn -> column (for this line and above exceptions)

kwburgess · 2018-07-20T23:06:57Z

src/main/net/sf/persist/NoTableMapping.java

+            }
+            return setterForFieldName.get();
+        }
+        return null;


add warn (similar to getter)

kwburgess · 2018-07-20T23:08:18Z

src/main/net/sf/persist/NoTableMapping.java

+            final Optional<Method> getterForFieldName = Optional.ofNullable(gettersMap.get(fieldName.get()));
+            if (!getterForFieldName.isPresent()) {
+                throw new NoSuchElementException(
+                    "Could not find getter for columnn with field name [" + fieldName + "]");


same comments as above (would love to understand the difference between "No Table Mapping" and "annotation table mapping " in persist library)

changes added back to persist branch

46b1545

samuellush changed the title ~~changes added back to persist branch~~ ENG-5818vPersistOnly added optionals Jul 18, 2018

samuellush requested a review from kwburgess July 18, 2018 00:03

kwburgess reviewed Jul 18, 2018

View reviewed changes

made changes corresponding to review comments

b3c0aa2

kwburgess reviewed Jul 18, 2018

View reviewed changes

kwburgess approved these changes Jul 18, 2018

View reviewed changes

made changes corresponding to review comments

4f1e720

custom classes added

376137a

samuellush requested a review from kwburgess July 20, 2018 21:38

kwburgess reviewed Jul 20, 2018

View reviewed changes

src/main/net/sf/persist/AnnotationTableMapping.java

}

return serializationTypeForFieldName.get();

}

return null;

Copy link

kwburgess Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use case?

kwburgess reviewed Jul 20, 2018

View reviewed changes

src/main/net/sf/persist/AnnotationTableMapping.java

}

return writerClassForFieldName.get();

}

return null;

Copy link

kwburgess Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use case?

kwburgess reviewed Jul 20, 2018

View reviewed changes

src/main/net/sf/persist/NoTableMapping.java

}

return setterForFieldName.get();

}

return null;

Copy link

kwburgess Jul 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add warn (similar to getter)

kwburgess reviewed Jul 20, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENG-5818vPersistOnly added optionals #1

ENG-5818vPersistOnly added optionals #1

samuellush commented Jul 17, 2018

kwburgess Jul 18, 2018

kwburgess Jul 18, 2018

kwburgess Jul 18, 2018

kwburgess Jul 18, 2018

kwburgess Jul 18, 2018

kwburgess Jul 18, 2018

kwburgess Jul 18, 2018

kwburgess Jul 18, 2018

kwburgess Jul 18, 2018

kwburgess Jul 18, 2018

kwburgess Jul 18, 2018

kwburgess Jul 18, 2018

kwburgess left a comment

samuellush commented Jul 18, 2018

kwburgess Jul 20, 2018

kwburgess Jul 20, 2018

kwburgess Jul 20, 2018

kwburgess Jul 20, 2018

kwburgess Jul 20, 2018

kwburgess Jul 20, 2018 •

edited

Loading

kwburgess Jul 20, 2018

kwburgess Jul 20, 2018

ENG-5818vPersistOnly added optionals #1

Are you sure you want to change the base?

ENG-5818vPersistOnly added optionals #1

Conversation

samuellush commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwburgess left a comment

Choose a reason for hiding this comment

samuellush commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwburgess Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwburgess Jul 20, 2018 •

edited

Loading