exceptionfactory commented on code in PR #8332: URL: https://github.com/apache/nifi/pull/8332#discussion_r1476872995
########## nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/field/TestObjectLocalDateTimeFieldConverter.java: ########## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.nifi.serialization.record.field; + +import org.junit.jupiter.api.Test; + +import java.time.Instant; +import java.time.LocalDateTime; +import java.time.ZoneId; +import java.util.Optional; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class TestObjectLocalDateTimeFieldConverter { + + @Test + public void testConvertTimestampMillis() { + final ObjectLocalDateTimeFieldConverter converter = new ObjectLocalDateTimeFieldConverter(); + final long now = System.currentTimeMillis(); + final Instant instant = Instant.ofEpochMilli(now); + final LocalDateTime expected = LocalDateTime.ofInstant(instant, ZoneId.systemDefault()); + + final LocalDateTime result = converter.convertField(now, Optional.empty(), "test"); + assertEquals(expected, result); + assertEquals(now, result.atZone(ZoneId.systemDefault()).toInstant().toEpochMilli()); + } + + @Test + public void testConvertTimestampMicros() { + final ObjectLocalDateTimeFieldConverter converter = new ObjectLocalDateTimeFieldConverter(); + final long now = System.currentTimeMillis(); + final long withMicros = now * 1000 + 567; + + final LocalDateTime result = converter.convertField(withMicros, Optional.empty(), "test"); + assertEquals(now, result.atZone(ZoneId.systemDefault()).toInstant().toEpochMilli()); + + final Instant resultInstant = result.atZone(ZoneId.systemDefault()).toInstant(); + final long expectedNanos = now % 1000 * 1_000_000 + 567_000; + assertEquals(expectedNanos, resultInstant.getNano()); + } + + @Test + public void testDoubleAsEpochSeconds() { + final ObjectLocalDateTimeFieldConverter converter = new ObjectLocalDateTimeFieldConverter(); + final long millis = System.currentTimeMillis(); + final double seconds = millis / 1000.0; + final double withMicros = seconds + 0.000567; + + final LocalDateTime result = converter.convertField(withMicros, Optional.empty(), "test"); + assertEquals(LocalDateTime.ofInstant(Instant.ofEpochMilli(millis), ZoneId.systemDefault()).toLocalDate(), result.toLocalDate()); + final double expectedNanos = (withMicros - (long) seconds) * 1_000_000_000; + assertEquals(expectedNanos, result.getNano(), 1000D); + } + + @Test + public void testDoubleAsEpochSecondsAsString() { + final ObjectLocalDateTimeFieldConverter converter = new ObjectLocalDateTimeFieldConverter(); + final long millis = System.currentTimeMillis(); + final double seconds = millis / 1000.0; + final double withMicros = seconds + 0.000567; + + final LocalDateTime result = converter.convertField(Double.toString(withMicros), Optional.empty(), "test"); + assertEquals(LocalDateTime.ofInstant(Instant.ofEpochMilli(millis), ZoneId.systemDefault()).toLocalDate(), result.toLocalDate()); Review Comment: This comparison is a bit difficult to follow, but it appears to be comparing only the `LocalDate` portion of `LocalDateTime`. Is there a reason for this comparison as opposed to the nanosecond comparison below? ########## nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/field/TestObjectLocalDateTimeFieldConverter.java: ########## @@ -0,0 +1,99 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.nifi.serialization.record.field; + +import org.junit.jupiter.api.Test; + +import java.time.Instant; +import java.time.LocalDateTime; +import java.time.ZoneId; +import java.util.Optional; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class TestObjectLocalDateTimeFieldConverter { + + @Test + public void testConvertTimestampMillis() { + final ObjectLocalDateTimeFieldConverter converter = new ObjectLocalDateTimeFieldConverter(); + final long now = System.currentTimeMillis(); + final Instant instant = Instant.ofEpochMilli(now); + final LocalDateTime expected = LocalDateTime.ofInstant(instant, ZoneId.systemDefault()); + + final LocalDateTime result = converter.convertField(now, Optional.empty(), "test"); + assertEquals(expected, result); + assertEquals(now, result.atZone(ZoneId.systemDefault()).toInstant().toEpochMilli()); Review Comment: Is there a particular reason for this comparison after comparing the expected value above? Converting to a different representation is effectively mutating the value, versus testing the `LocalDateTime` conversion on its own. ########## nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/field/ObjectLocalDateTimeFieldConverter.java: ########## @@ -51,10 +51,13 @@ public LocalDateTime convertField(final Object field, final Optional<String> pat final Instant instant = Instant.ofEpochMilli(date.getTime()); return ofInstant(instant); } - if (field instanceof Number) { - final Number number = (Number) field; - final Instant instant = Instant.ofEpochMilli(number.longValue()); - return ofInstant(instant); + if (field instanceof final Number number) { + // If value is a floating point number, we consider it as seconds since epoch plus a decimal part for fractions of a second. + if (field instanceof Double || field instanceof Float) { + // Use long value as seconds; use everything after it as fractions of a second and multiply by 1_000_000_000 to get nanoseconds. + return toLocalDateTime(number.longValue(), (long) ((number.doubleValue() - number.longValue()) * 1_000_000_000)); Review Comment: Recommend breaking out nanosecond assignment to a separate line for readability, as it is a bit difficult to follow the levels of nested calculation. ########## nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/java/org/apache/nifi/processors/standard/PutDatabaseRecordIT.java: ########## @@ -0,0 +1,312 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.nifi.processors.standard; + +import org.apache.nifi.dbcp.DBCPConnectionPool; +import org.apache.nifi.dbcp.utils.DBCPProperties; +import org.apache.nifi.json.JsonTreeReader; +import org.apache.nifi.reporting.InitializationException; +import org.apache.nifi.serialization.DateTimeUtils; +import org.apache.nifi.util.TestRunner; +import org.apache.nifi.util.TestRunners; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.testcontainers.containers.PostgreSQLContainer; + +import java.sql.Connection; +import java.sql.Date; +import java.sql.DriverManager; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.ResultSetMetaData; +import java.sql.SQLException; +import java.sql.Timestamp; +import java.time.Instant; +import java.time.LocalDateTime; +import java.time.Month; +import java.time.temporal.ChronoUnit; +import java.util.HashMap; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +@SuppressWarnings("resource") +public class PutDatabaseRecordIT { + + private static PostgreSQLContainer<?> postgres; + private TestRunner runner; + + + @BeforeAll + public static void startPostgres() { + postgres = new PostgreSQLContainer<>("postgres:9.6.12") + .withInitScript("PutDatabaseRecordIT/create-person-table.sql"); + postgres.start(); + } + + @AfterAll + public static void cleanup() { + if (postgres != null) { + postgres.close(); + postgres = null; + } + } + + @BeforeEach + public void setup() throws InitializationException, SQLException { + truncateTable(); + + runner = TestRunners.newTestRunner(PutDatabaseRecord.class); + final DBCPConnectionPool connectionPool = new DBCPConnectionPool(); + runner.addControllerService("connectionPool", connectionPool); + runner.setProperty(connectionPool, DBCPProperties.DATABASE_URL, postgres.getJdbcUrl()); + runner.setProperty(connectionPool, DBCPProperties.DB_USER, postgres.getUsername()); + runner.setProperty(connectionPool, DBCPProperties.DB_PASSWORD, postgres.getPassword()); + runner.setProperty(connectionPool, DBCPProperties.DB_DRIVERNAME, postgres.getDriverClassName()); + runner.enableControllerService(connectionPool); + + final JsonTreeReader jsonReader = new JsonTreeReader(); + runner.addControllerService("json-reader", jsonReader); + runner.setProperty(jsonReader, DateTimeUtils.DATE_FORMAT, "yyyy-MM-dd"); + runner.setProperty(jsonReader, DateTimeUtils.TIMESTAMP_FORMAT, "yyyy-MM-dd HH:mm:ss.SSSSSS"); + runner.enableControllerService(jsonReader); + + runner.setProperty(PutDatabaseRecord.RECORD_READER_FACTORY, "json-reader"); + runner.setProperty(PutDatabaseRecord.DBCP_SERVICE, "connectionPool"); + + runner.setProperty(PutDatabaseRecord.TABLE_NAME, "person"); + runner.setProperty(PutDatabaseRecord.DB_TYPE, "PostgreSQL"); + runner.setProperty(PutDatabaseRecord.STATEMENT_TYPE, "INSERT"); + } + + + @Test + public void testSimplePut() throws SQLException { + runner.enqueue(""" + { + "name": "John Doe", + "age": 50, + "favorite_color": "blue" + } + """); + runner.run(); + runner.assertAllFlowFilesTransferred(PutDatabaseRecord.REL_SUCCESS, 1); + + final Map<String, Object> results = getResults(); + assertEquals("John Doe", results.get("name")); + assertEquals(50, results.get("age")); + assertEquals("blue", results.get("favorite_color")); + } + + @Test + public void testWithDate() throws SQLException { + runner.enqueue(""" + { + "name": "John Doe", + "age": 50, + "dob": "1975-01-01" + } + """); + runner.run(); + runner.assertAllFlowFilesTransferred(PutDatabaseRecord.REL_SUCCESS, 1); + + + final Map<String, Object> results = getResults(); + assertEquals("John Doe", results.get("name")); + assertEquals(50, results.get("age")); Review Comment: These comparisons are repeated in most of the test methods, along with the same data. It looks like the test methods could be refactored so that the shared behavior is not repeated. That would make this test more maintainable, avoid some of the duplication, and highlight what is unique about the particular test method (Date, Timestamp, Milliseconds, Microseconds, etc). ########## nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/field/ObjectLocalDateTimeFieldConverter.java: ########## @@ -67,22 +70,49 @@ public LocalDateTime convertField(final Object field, final Optional<String> pat try { return LocalDateTime.parse(string, formatter); } catch (final DateTimeParseException e) { - throw new FieldConversionException(LocalDateTime.class, field, name, e); + return tryParseAsNumber(string, name); } } else { - try { - final long number = Long.parseLong(string); - final Instant instant = Instant.ofEpochMilli(number); - return ofInstant(instant); - } catch (final NumberFormatException e) { - throw new FieldConversionException(LocalDateTime.class, field, name, e); - } + return tryParseAsNumber(string, name); } } throw new FieldConversionException(LocalDateTime.class, field, name); } + private LocalDateTime tryParseAsNumber(final String value, final String fieldName) { + // If decimal, treat as a double and convert to seconds and nanoseconds. + if (value.contains(".")) { + final double number = Double.parseDouble(value); + return toLocalDateTime((long) number, (long) ((number - (long) number) * 1_000_000_000)); Review Comment: Similar to above, recommend assigning nanosPastSecond separately for readability. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
