From 74b072518a7641c34e6aa8d076f7a841bccec0dc Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 16:26:00 +0000 Subject: [PATCH 1/6] Test the data update logging, the If-Modified-Since time and the licence key check The tests read the messages and levels from the Logging section of the data update specification, check that update on start-up works with auto update off and that the file system watcher follows the auto update switch, and that an engine whose update URL is the Distributor's is refused without a licence key. Tests that built an engine with the defaults now switch auto update off or give a key, as the specification's defaults table says. --- ...DetectionOnPremisePipelineBuilderTest.java | 8 + ...elligenceOnPremisePipelineBuilderTest.java | 7 + ...OnPremiseAspectEngineBuilderBaseTests.java | 103 ++++++ .../DataUpdateServiceLoggingTests.java | 336 ++++++++++++++++++ .../services/DataUpdateServiceTests.java | 15 +- 5 files changed, 465 insertions(+), 4 deletions(-) create mode 100644 pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceLoggingTests.java diff --git a/device-detection/device-detection/src/test/java/fiftyone/devicedetection/DeviceDetectionOnPremisePipelineBuilderTest.java b/device-detection/device-detection/src/test/java/fiftyone/devicedetection/DeviceDetectionOnPremisePipelineBuilderTest.java index 21b9e5bd4..aed639665 100644 --- a/device-detection/device-detection/src/test/java/fiftyone/devicedetection/DeviceDetectionOnPremisePipelineBuilderTest.java +++ b/device-detection/device-detection/src/test/java/fiftyone/devicedetection/DeviceDetectionOnPremisePipelineBuilderTest.java @@ -64,6 +64,7 @@ public void setup() throws Exception { when(loggerFactory.getLogger(anyString())).thenReturn(logger); builder = new DeviceDetectionPipelineBuilder(loggerFactory) .useOnPremise(datafile.getAbsolutePath(), false) + .setAutoUpdate(false) .setShareUsage(false); } @@ -122,6 +123,7 @@ public void setEngineData_PropertyPropagatedToDataFileConfiguration() throws Exc try (Pipeline pipeline = new DeviceDetectionPipelineBuilder(loggerFactory) .useOnPremise(new byte[]{}, Enums.DeviceDetectionAlgorithm.Hash) .setEngineData(data) + .setAutoUpdate(false) .setShareUsage(false) .build()) { DataFileConfiguration dataFileConfiguration = getDataFileConfiguration(pipeline); @@ -144,6 +146,7 @@ public void setEngineData_FilenameIsSet_PropertyNotPropagated() throws Exception public void setShareUsage_False_SharedFlowElementNotAdded() throws Exception { try (Pipeline pipeline = new DeviceDetectionPipelineBuilder(loggerFactory) .useOnPremise(datafile.getAbsolutePath(), false) + .setAutoUpdate(false) .setShareUsage(false) .build()) { List flowElements = pipeline.getFlowElements(); @@ -156,6 +159,7 @@ public void setShareUsage_False_SharedFlowElementNotAdded() throws Exception { public void setShareUsage_True_SharedFlowElementAdded() throws Exception { try (Pipeline pipeline = new DeviceDetectionPipelineBuilder(loggerFactory) .useOnPremise(datafile.getAbsolutePath(), false) + .setAutoUpdate(false) .setShareUsage(true) .build()) { List flowElements = pipeline.getFlowElements(); @@ -168,6 +172,7 @@ public void setShareUsage_True_SharedFlowElementAdded() throws Exception { public void setAutoUpdate_PropertyPropagatedToDataFileConfiguration() throws Exception { try (Pipeline pipeline = builder .setAutoUpdate(true) + .setDataUpdateLicenseKey("licence_key") .build()) { DataFileConfiguration dataFileConfiguration = getDataFileConfiguration(pipeline); assertTrue(dataFileConfiguration.getAutomaticUpdatesEnabled()); @@ -189,6 +194,7 @@ public void setDataUpdateService_RegisterDataFileTriggered() throws Exception { DataUpdateServiceDefault dataUpdateServiceDefault = mock(DataUpdateServiceDefault.class); try (Pipeline ignored = builder .setAutoUpdate(true) + .setDataUpdateLicenseKey("licence_key") .setDataUpdateService(dataUpdateServiceDefault) .build()) { verify(dataUpdateServiceDefault).registerDataFile(any()); @@ -199,6 +205,8 @@ public void setDataUpdateService_RegisterDataFileTriggered() throws Exception { public void setDataUpdateOnStartup_PropertyPropagatedToDataFileConfiguration() throws Exception { try (Pipeline pipeline = builder .setDataUpdateOnStartup(true) + .setDataUpdateLicenseKey("licence_key") + .setDataUpdateService(mock(DataUpdateServiceDefault.class)) .build()) { DataFileConfiguration dataFileConfiguration = getDataFileConfiguration(pipeline); assertTrue(dataFileConfiguration.getUpdateOnStartup()); diff --git a/ip-intelligence/ip-intelligence/src/test/java/fiftyone/ipintelligence/IPIntelligenceOnPremisePipelineBuilderTest.java b/ip-intelligence/ip-intelligence/src/test/java/fiftyone/ipintelligence/IPIntelligenceOnPremisePipelineBuilderTest.java index d220eb776..9e37ada63 100644 --- a/ip-intelligence/ip-intelligence/src/test/java/fiftyone/ipintelligence/IPIntelligenceOnPremisePipelineBuilderTest.java +++ b/ip-intelligence/ip-intelligence/src/test/java/fiftyone/ipintelligence/IPIntelligenceOnPremisePipelineBuilderTest.java @@ -65,6 +65,7 @@ public void setup() throws Exception { when(loggerFactory.getLogger(anyString())).thenReturn(logger); builder = new IPIntelligencePipelineBuilder(loggerFactory) .useOnPremise(datafile.getAbsolutePath(), false) + .setAutoUpdate(false) .setShareUsage(false); } @@ -121,6 +122,7 @@ public void setFilename_OtherExtension_ExceptionThrown() { public void setShareUsage_False_SharedFlowElementNotAdded() throws Exception { try (Pipeline pipeline = new IPIntelligencePipelineBuilder(loggerFactory) .useOnPremise(datafile.getAbsolutePath(), false) + .setAutoUpdate(false) .setShareUsage(false) .build()) { List flowElements = pipeline.getFlowElements(); @@ -133,6 +135,7 @@ public void setShareUsage_False_SharedFlowElementNotAdded() throws Exception { public void setShareUsage_True_SharedFlowElementAdded() throws Exception { try (Pipeline pipeline = new IPIntelligencePipelineBuilder(loggerFactory) .useOnPremise(datafile.getAbsolutePath(), false) + .setAutoUpdate(false) .setShareUsage(true) .build()) { List flowElements = pipeline.getFlowElements(); @@ -145,6 +148,7 @@ public void setShareUsage_True_SharedFlowElementAdded() throws Exception { public void setAutoUpdate_PropertyPropagatedToDataFileConfiguration() throws Exception { try (Pipeline pipeline = builder .setAutoUpdate(true) + .setDataUpdateLicenseKey("licence_key") .build()) { DataFileConfiguration dataFileConfiguration = getDataFileConfiguration(pipeline); assertTrue(dataFileConfiguration.getAutomaticUpdatesEnabled()); @@ -166,6 +170,7 @@ public void setDataUpdateService_RegisterDataFileTriggered() throws Exception { DataUpdateServiceDefault dataUpdateServiceDefault = mock(DataUpdateServiceDefault.class); try (Pipeline ignored = builder .setAutoUpdate(true) + .setDataUpdateLicenseKey("licence_key") .setDataUpdateService(dataUpdateServiceDefault) .build()) { verify(dataUpdateServiceDefault).registerDataFile(any()); @@ -176,6 +181,8 @@ public void setDataUpdateService_RegisterDataFileTriggered() throws Exception { public void setDataUpdateOnStartup_PropertyPropagatedToDataFileConfiguration() throws Exception { try (Pipeline pipeline = builder .setDataUpdateOnStartup(true) + .setDataUpdateLicenseKey("licence_key") + .setDataUpdateService(mock(DataUpdateServiceDefault.class)) .build()) { DataFileConfiguration dataFileConfiguration = getDataFileConfiguration(pipeline); assertTrue(dataFileConfiguration.getUpdateOnStartup()); diff --git a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/FiftyOneDegreesOnPremiseAspectEngineBuilderBaseTests.java b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/FiftyOneDegreesOnPremiseAspectEngineBuilderBaseTests.java index c1ff832dc..9caab246c 100644 --- a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/FiftyOneDegreesOnPremiseAspectEngineBuilderBaseTests.java +++ b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/FiftyOneDegreesOnPremiseAspectEngineBuilderBaseTests.java @@ -22,6 +22,7 @@ package fiftyone.pipeline.engines.fiftyone.flowelements; +import fiftyone.pipeline.core.exceptions.PipelineConfigurationException; import fiftyone.pipeline.engines.Constants; import fiftyone.pipeline.engines.data.AspectData; import fiftyone.pipeline.engines.data.AspectEngineDataFile; @@ -38,7 +39,10 @@ import java.util.List; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.*; @@ -118,6 +122,7 @@ public void DataFile_SetEngine() throws Exception { ImplementedBuilder builder = new ImplementedBuilder( LoggerFactory.getILoggerFactory(), updateService); + builder.setAutoUpdate(false); builder.build("", false); assertNotNull(engine.getDataFileMetaData()); @@ -145,4 +150,102 @@ public void DataFile_UrlFormatter() throws Exception { "https://distributor.51degrees.com/api/v2/download?LicenseKeys="+licenseKey+"&Download=True&Type="+downloadType, engine.getDataFileMetaData().getFormattedUrl()); } + + private ImplementedBuilder newBuilder() { + return new ImplementedBuilder( + LoggerFactory.getILoggerFactory(), + updateService); + } + + /** + * The data file is registered with the update service when update on + * start-up is on, even though auto update is off. + */ + @Test + public void UpdateOnStartup_AutoUpdateOff_DataFileRegistered() + throws Exception { + ImplementedBuilder builder = newBuilder() + .setAutoUpdate(false) + .setDataUpdateOnStartup(true) + .setDataUpdateLicenseKey(licenseKey); + + builder.build("", false); + + verify(updateService, times(1)).registerDataFile(any()); + } + + @Test + public void AutoUpdateAndUpdateOnStartupOff_DataFileNotRegistered() + throws Exception { + ImplementedBuilder builder = newBuilder() + .setAutoUpdate(false) + .setDataUpdateOnStartup(false); + + builder.build("", false); + + verify(updateService, never()).registerDataFile(any()); + } + + /** + * The default update URL is the Distributor's, which serves nothing + * without a license key. + */ + @Test + public void AutoUpdate_DefaultUrlNoLicenseKey_BuildRefused() { + ImplementedBuilder builder = newBuilder().setAutoUpdate(true); + + PipelineConfigurationException e = assertThrows( + PipelineConfigurationException.class, + () -> builder.build("", false)); + + assertTrue(e.getMessage().contains("setAutoUpdate(false)")); + assertFalse(e.getMessage().contains("setDataUpdateOnStartup(false)")); + verify(updateService, never()).registerDataFile(any()); + } + + @Test + public void UpdateOnStartup_DefaultUrlNoLicenseKey_BuildRefused() { + ImplementedBuilder builder = newBuilder() + .setAutoUpdate(false) + .setDataUpdateOnStartup(true); + + PipelineConfigurationException e = assertThrows( + PipelineConfigurationException.class, + () -> builder.build("", false)); + + assertTrue(e.getMessage().contains("setDataUpdateOnStartup(false)")); + assertFalse(e.getMessage().contains("setAutoUpdate(false)")); + } + + @Test + public void AutoUpdate_LicenseKeySet_BuildAllowed() throws Exception { + ImplementedBuilder builder = newBuilder() + .setAutoUpdate(true) + .setDataUpdateLicenseKey(licenseKey); + + builder.build("", false); + + verify(updateService, times(1)).registerDataFile(any()); + } + + @Test + public void AutoUpdate_OtherUrlNoLicenseKey_BuildAllowed() + throws Exception { + ImplementedBuilder builder = newBuilder() + .setAutoUpdate(true) + .setDataUpdateUrl("https://example.com/data.file"); + + builder.build("", false); + + verify(updateService, times(1)).registerDataFile(any()); + } + + @Test + public void AutoUpdateOff_NoLicenseKey_BuildAllowed() throws Exception { + ImplementedBuilder builder = newBuilder().setAutoUpdate(false); + + builder.build("", false); + + verify(updateService, never()).registerDataFile(any()); + } } diff --git a/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceLoggingTests.java b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceLoggingTests.java new file mode 100644 index 000000000..dcdd6b569 --- /dev/null +++ b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceLoggingTests.java @@ -0,0 +1,336 @@ +/* ********************************************************************* + * This Original Work is copyright of 51 Degrees Mobile Experts Limited. + * Copyright 2026 51 Degrees Mobile Experts Limited, Davidson House, + * Forbury Square, Reading, Berkshire, United Kingdom RG1 3EU. + * + * This Original Work is licensed under the European Union Public Licence + * (EUPL) v.1.2 and is subject to its terms as set out below. + * + * If a copy of the EUPL was not distributed with this file, You can obtain + * one at https://opensource.org/licenses/EUPL-1.2. + * + * The 'Compatible Licences' set out in the Appendix to the EUPL (as may be + * amended by the European Commission) shall be deemed incompatible for + * the purposes of the Work and the provisions of the compatibility + * clause in Article 5 of the EUPL shall not apply. + * + * If using the Work as, or as part of, a network application, by + * including the attribution notice(s) required under Article 5 of the EUPL + * in the end user terms of the application under an appropriate heading, + * such notice(s) shall fulfill the requirements of that article. + * ********************************************************************* */ + +package fiftyone.pipeline.engines.services; + +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +import fiftyone.common.wrappers.io.FileWrapperFactoryDefault; +import fiftyone.pipeline.engines.configuration.DataFileConfigurationDefault; +import fiftyone.pipeline.engines.data.AspectData; +import fiftyone.pipeline.engines.data.AspectEngineDataFileDefault; +import fiftyone.pipeline.engines.data.AspectPropertyMetaData; +import fiftyone.pipeline.engines.flowelements.OnPremiseAspectEngine; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.slf4j.LoggerFactory; + +import java.io.ByteArrayInputStream; +import java.io.File; +import java.io.IOException; +import java.net.HttpURLConnection; +import java.net.URL; +import java.util.ArrayList; +import java.util.List; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * The messages and levels in the Logging section of the data update + * specification, and the If-Modified-Since time the update URL is sent. + * @see Specification + */ +@SuppressWarnings("unchecked") +public class DataUpdateServiceLoggingTests { + + private static final String UPDATE_URL = "http://test.com"; + // 13 September 2020, 12:26:40 UTC. + private static final long FILE_MODIFIED_MILLIS = 1600000000000L; + + private ch.qos.logback.classic.Logger logger; + private ListAppender appender; + private HttpClient httpClient; + private HttpURLConnection connection; + private DataUpdateServiceDefault dataUpdate; + private OnPremiseAspectEngine engine; + private String engineType; + private File dataFile; + + @Before + public void init() throws IOException { + logger = (ch.qos.logback.classic.Logger) LoggerFactory.getLogger( + "DataUpdateServiceLoggingTests"); + logger.setLevel(Level.INFO); + appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + + connection = mock(HttpURLConnection.class); + httpClient = mock(HttpClient.class); + when(httpClient.connect(any(URL.class))).thenReturn(connection); + // The futures are real but their first run is a day away. + dataUpdate = new DataUpdateServiceDefault( + logger, httpClient, new FileWrapperFactoryDefault(), null); + + engine = mock(OnPremiseAspectEngine.class); + engineType = engine.getClass().getSimpleName(); + dataFile = File.createTempFile("logging-test", ".tmp"); + assertTrue(dataFile.setLastModified(FILE_MODIFIED_MILLIS)); + } + + @After + public void cleanup() { + dataUpdate.close(); + logger.detachAppender(appender); + dataFile.delete(); + } + + private AspectEngineDataFileDefault newDataFile( + boolean inMemory, + boolean autoUpdate, + boolean updateOnStartup) { + DataFileConfigurationDefault config = new DataFileConfigurationDefault(); + config.setAutomaticUpdatesEnabled(autoUpdate); + config.setUpdateOnStartup(updateOnStartup); + config.setFileSystemWatcherEnabled(false); + config.setDataUpdateUrl(UPDATE_URL); + config.setVerifyMd5(false); + config.setDecompressContent(false); + config.setPollingIntervalSeconds(86400); + config.setMaxRandomisationSeconds(0); + if (inMemory == false) { + config.setDataFilePath(dataFile.getAbsolutePath()); + } + AspectEngineDataFileDefault file = new AspectEngineDataFileDefault(); + file.setIdentifier("default"); + file.setEngine(engine); + file.setConfiguration(config); + file.setTempDataDirPath(System.getProperty("java.io.tmpdir")); + when(engine.getDataFileMetaData()).thenReturn(file); + return file; + } + + private List logged(Level level) { + List messages = new ArrayList<>(); + for (ILoggingEvent event : appender.list) { + if (event.getLevel() == level) { + messages.add(event.getFormattedMessage()); + } + } + return messages; + } + + private void assertLogged(Level level, String message) { + List messages = logged(level); + assertTrue( + "Expected '" + message + "' at " + level + " but got " + messages, + messages.contains(message)); + } + + @Test + public void IfModifiedSince_NoPublishedDate_UsesFileLastModified() + throws Exception { + when(connection.getResponseCode()) + .thenReturn(HttpURLConnection.HTTP_NOT_MODIFIED); + AspectEngineDataFileDefault file = newDataFile(false, false, false); + file.getConfiguration().setVerifyModifiedSince(true); + + dataUpdate.checkForUpdate(engine); + + verify(connection).setIfModifiedSince(FILE_MODIFIED_MILLIS); + } + + @Test + public void IfModifiedSince_PublishedDate_UsedInsteadOfFileLastModified() + throws Exception { + when(connection.getResponseCode()) + .thenReturn(HttpURLConnection.HTTP_NOT_MODIFIED); + AspectEngineDataFileDefault file = newDataFile(false, false, false); + file.getConfiguration().setVerifyModifiedSince(true); + file.setDataPublishedDateTime(new java.util.Date(1500000000000L)); + + dataUpdate.checkForUpdate(engine); + + verify(connection).setIfModifiedSince(1500000000000L); + } + + @Test + public void Check_NotModified_LogsTheSpecifiedMessages() throws Exception { + when(connection.getResponseCode()) + .thenReturn(HttpURLConnection.HTTP_NOT_MODIFIED); + AspectEngineDataFileDefault file = newDataFile(false, false, false); + file.getConfiguration().setVerifyModifiedSince(true); + + dataUpdate.checkForUpdate(engine); + + assertLogged(Level.INFO, "Checking for update"); + assertLogged(Level.INFO, "Checking for update from '" + UPDATE_URL + + "' for Engine '" + engineType + "'"); + assertLogged(Level.INFO, "No data newer than 2020-09-13T12:26:40Z " + + "found at '" + UPDATE_URL + "' for Engine '" + engineType + "'"); + } + + @Test + public void Check_Downloaded_LogsDownloadAndRefresh() throws Exception { + when(connection.getResponseCode()) + .thenReturn(HttpURLConnection.HTTP_OK); + when(connection.getInputStream()) + .thenReturn(new ByteArrayInputStream(new byte[]{1, 2, 3})); + newDataFile(true, false, false); + + dataUpdate.checkForUpdate(engine); + + assertLogged(Level.INFO, "Downloaded new data from '" + UPDATE_URL + + "' for Engine '" + engineType + "'"); + assertLogged(Level.INFO, "Attempting to refresh Engine '" + + engineType + "' with new data"); + } + + @Test + public void UpdateFromMemory_LogsRefresh() { + AspectEngineDataFileDefault file = newDataFile(true, false, false); + + dataUpdate.updateFromMemory(file, new byte[]{1, 2, 3}); + + assertLogged(Level.INFO, "Attempting to refresh Engine '" + + engineType + "' with new data"); + } + + @Test + public void UpdateFromMemory_RefreshFails_LogsErrorWithDetail() + throws Exception { + AspectEngineDataFileDefault file = newDataFile(true, false, false); + doThrow(new RuntimeException("no room")) + .when(engine).refreshData(anyString(), any(byte[].class)); + + dataUpdate.updateFromMemory(file, new byte[]{1, 2, 3}); + + assertLogged(Level.ERROR, "An error occurred while applying a data " + + "file update to " + engineType + ". Error detail: no room"); + } + + @Test + public void UpdateOnStartup_LogsUpdatingOnStartUp() throws Exception { + when(connection.getResponseCode()) + .thenReturn(HttpURLConnection.HTTP_NOT_MODIFIED); + AspectEngineDataFileDefault file = newDataFile(false, false, true); + + dataUpdate.registerDataFile(file); + + assertLogged(Level.INFO, "Updating on start-up"); + } + + @Test + public void UpdateOnStartup_AutoUpdateOff_StartsNoTimer() throws Exception { + when(connection.getResponseCode()) + .thenReturn(HttpURLConnection.HTTP_NOT_MODIFIED); + AspectEngineDataFileDefault file = newDataFile(false, false, true); + + dataUpdate.registerDataFile(file); + + assertEquals(null, file.getFuture()); + } + + @Test + public void FileSystemWatcher_LogsCreatingFileSystemWatcher() { + AspectEngineDataFileDefault file = newDataFile(false, true, false); + file.getConfiguration().setFileSystemWatcherEnabled(true); + + dataUpdate.registerDataFile(file); + + assertLogged(Level.INFO, "Creating file system watcher"); + assertNotNull(file.getConfiguration().getWatchKey()); + } + + @Test + public void ConnectFails_Manual_WarnsWithoutRetryText() throws Exception { + when(httpClient.connect(any(URL.class))) + .thenThrow(new IOException("refused")); + newDataFile(false, false, false); + + dataUpdate.checkForUpdate(engine); + + assertLogged(Level.WARN, "An error occurred when connecting to " + + UPDATE_URL + " in order to check for data file updates for " + + engineType + ". Error detail: refused"); + } + + @Test + public void ConnectFails_Automatic_WarnsThatUpdateWillBeTriedAgain() + throws Exception { + when(httpClient.connect(any(URL.class))) + .thenThrow(new IOException("refused")); + AspectEngineDataFileDefault file = newDataFile(false, true, true); + + dataUpdate.registerDataFile(file); + + assertLogged(Level.WARN, "An error occurred when connecting to " + + UPDATE_URL + " in order to check for data file updates for " + + engineType + ". Update will be attempted again later. " + + "Error detail: refused"); + } + + @Test + public void ConnectFails_AutoUpdateOff_DoesNotPromiseAnotherTry() + throws Exception { + when(httpClient.connect(any(URL.class))) + .thenThrow(new IOException("refused")); + AspectEngineDataFileDefault file = newDataFile(false, false, true); + + dataUpdate.registerDataFile(file); + + assertLogged(Level.WARN, "An error occurred when connecting to " + + UPDATE_URL + " in order to check for data file updates for " + + engineType + ". Error detail: refused"); + } + + @Test + public void Md5DoesNotMatch_WarnsAboutIntegrityCheck() throws Exception { + when(connection.getResponseCode()) + .thenReturn(HttpURLConnection.HTTP_OK); + when(connection.getInputStream()) + .thenReturn(new ByteArrayInputStream(new byte[]{1, 2, 3})); + when(connection.getHeaderField("Content-MD5")).thenReturn("0"); + AspectEngineDataFileDefault file = newDataFile(true, false, false); + file.getConfiguration().setVerifyMd5(true); + + dataUpdate.checkForUpdate(engine); + + assertLogged(Level.WARN, "An error occurred during the integrity " + + "check of new data file for " + engineType + ". Error detail: " + + "the MD5 hash of the download does not match the Content-MD5 " + + "header"); + } + + @Test + public void NoTemporaryFilePath_LogsError() { + AspectEngineDataFileDefault file = newDataFile(false, false, false); + file.setTempDataDirPath(null); + when(engine.getTempDataDirPath()).thenReturn(null); + + dataUpdate.checkForUpdate(engine); + + assertLogged(Level.ERROR, "The data file 'default' is checking for " + + "updates but does not have a temporary file path configured."); + } +} diff --git a/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceTests.java b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceTests.java index 52176409e..2106f669e 100644 --- a/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceTests.java +++ b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceTests.java @@ -371,7 +371,12 @@ public void DataUpdateService_Register_FileSystemWatcher() throws IOException { dataUpdate.registerDataFile(file); // Assert - assertNotNull(config.getWatchKey()); + // The automatic update switch governs the file system watcher. + if (autoUpdateEnabled) { + assertNotNull(config.getWatchKey()); + } else { + assertNull(config.getWatchKey()); + } } finally { // Make sure we tidy up the temp file. File tempFile = new File(tempData); @@ -884,10 +889,12 @@ public void call(Object sender, DataUpdateService.DataUpdateCompleteArgs args) { // reconfigured to try again later. verify(futureFactory, atLeast(1)) .schedule(any(Runnable.class), anyLong()); - // We expect one error to be logged so make sure it's + // We expect one warning to be logged so make sure it's // ignored in cleanup and verify its presence. - ignoreErrors = 1; - assertEquals(1, logger.errorsLogged.size()); + ignoreWranings = 1; + assertEquals(1, logger.warningsLogged.size()); + assertTrue(logger.warningsLogged.get(0).contains(errorText)); + assertEquals(0, logger.errorsLogged.size()); assertEquals(AUTO_UPDATE_HTTPS_ERR, completeEventArgs[0].getStatus()); } finally { From b989bc4a3b4bc6a17cc6c8581611fab99ec97e39 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 16:26:02 +0000 Subject: [PATCH 2/6] Make the data update follow the specification Send the data file's last modified time as If-Modified-Since when the file has no publication date. Register the data file with the update service when update on start-up or auto update is on, and let the auto update switch govern the file system watcher. Log the actions and errors with the messages and levels in the Logging section of the data update specification. Refuse to build with update on start-up or auto update on, the Distributor's URL and no licence key, naming the option to turn off. --- ...tyOneOnPremiseAspectEngineBuilderBase.java | 12 ++ .../OnPremiseAspectEngineBuilderBase.java | 48 ++++- .../services/DataUpdateServiceDefault.java | 168 +++++++++++++----- 3 files changed, 186 insertions(+), 42 deletions(-) diff --git a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/FiftyOneOnPremiseAspectEngineBuilderBase.java b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/FiftyOneOnPremiseAspectEngineBuilderBase.java index bcd634359..b6ea858ec 100644 --- a/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/FiftyOneOnPremiseAspectEngineBuilderBase.java +++ b/pipeline/pipeline.engines.fiftyone/src/main/java/fiftyone/pipeline/engines/fiftyone/flowelements/FiftyOneOnPremiseAspectEngineBuilderBase.java @@ -22,6 +22,7 @@ package fiftyone.pipeline.engines.fiftyone.flowelements; +import fiftyone.pipeline.engines.configuration.DataFileConfiguration; import fiftyone.pipeline.engines.data.AspectEngineDataFile; import fiftyone.pipeline.engines.fiftyone.data.FiftyOneDataFile; import fiftyone.pipeline.engines.fiftyone.data.FiftyOneDataFileDefault; @@ -80,6 +81,17 @@ public FiftyOneOnPremiseAspectEngineBuilderBase( setDataUpdateUrlFormatter(new FiftyOneUrlFormatter()); } + /** + * The Distributor serves nothing without a license key, so a key is + * required while the data update URL is the Distributor's. + * @param dataFileConfig the data file configuration to check + * @return true if the data update URL is the Distributor's + */ + @Override + protected boolean isLicenseKeyRequired(DataFileConfiguration dataFileConfig) { + return DATA_UPDATE_URL.equals(dataFileConfig.getDataUpdateUrl()); + } + /** * Create a new empty data file instance to be populated with the details of * the data file to be used. diff --git a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/flowelements/OnPremiseAspectEngineBuilderBase.java b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/flowelements/OnPremiseAspectEngineBuilderBase.java index 37c9dc7c9..c5a410a29 100644 --- a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/flowelements/OnPremiseAspectEngineBuilderBase.java +++ b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/flowelements/OnPremiseAspectEngineBuilderBase.java @@ -23,6 +23,7 @@ package fiftyone.pipeline.engines.flowelements; import fiftyone.pipeline.annotations.DefaultValue; +import fiftyone.pipeline.core.exceptions.PipelineConfigurationException; import fiftyone.pipeline.engines.Constants; import fiftyone.pipeline.engines.configuration.DataFileConfiguration; import fiftyone.pipeline.engines.data.AspectEngineDataFile; @@ -187,22 +188,65 @@ protected void preCreateEngine() { dataFile.setConfiguration(dataFileConfig); dataFile.setTempDataDirPath(tempDir); - if (dataFileConfig.getAutomaticUpdatesEnabled()) { + boolean autoUpdate = dataFileConfig.getAutomaticUpdatesEnabled(); + boolean updateOnStartup = dataFileConfig.getUpdateOnStartup(); + if (autoUpdate || updateOnStartup) { + String features = (autoUpdate ? "auto update" : "") + + (autoUpdate && updateOnStartup ? " and " : "") + + (updateOnStartup ? "update on startup" : ""); if (dataUpdateService == null) { throw new RuntimeException( - "Auto update enabled by data update service does not exist. " + + "The data update service does not exist but is " + + "needed for " + features + ". " + "This can be corrected by passing an DataUpdateService " + "instance to the engine builder constructor. " + "If building from configuration, this can be corrected " + "by adding an DataUpdateService instance to the " + "the pipeline builder via addService() method."); } + if (isLicenseKeyRequired(dataFileConfig) && + hasLicenseKey(dataFileConfig) == false) { + throw new PipelineConfigurationException( + "No license key is set, and the data update URL " + + "serves nothing without one, so the engine cannot " + + "be built with " + features + " on. Set a license " + + "key with setDataUpdateLicenseKey, or turn off " + + (autoUpdate ? "auto update with setAutoUpdate(false)" : "") + + (autoUpdate && updateOnStartup ? " and " : "") + + (updateOnStartup ? + "update on startup with setDataUpdateOnStartup(false)" : "") + + "."); + } dataUpdateService.registerDataFile(dataFile); } dataFiles.add(dataFile); } } + /** + * Whether the data update URL in the configuration serves nothing + * without a license key. Engines whose update source needs a key + * override this, and the engine then refuses to build with a data update + * option on and no key set. + * @param dataFileConfig the data file configuration to check + * @return true if the update source needs a license key + */ + protected boolean isLicenseKeyRequired(DataFileConfiguration dataFileConfig) { + return false; + } + + private static boolean hasLicenseKey(DataFileConfiguration dataFileConfig) { + List keys = dataFileConfig.getDataUpdateLicenseKeys(); + if (keys != null) { + for (String key : keys) { + if (key != null && key.isEmpty() == false) { + return true; + } + } + } + return false; + } + @SuppressWarnings("unchecked") @Override protected void configureEngine(TEngine engine) throws Exception { diff --git a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/DataUpdateServiceDefault.java b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/DataUpdateServiceDefault.java index 007b21211..5eb514cbd 100644 --- a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/DataUpdateServiceDefault.java +++ b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/DataUpdateServiceDefault.java @@ -45,6 +45,7 @@ import java.net.HttpURLConnection; import java.net.URI; import java.nio.file.*; +import java.time.Instant; import java.security.DigestInputStream; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; @@ -193,6 +194,7 @@ private AutoUpdateStatus checkForUpdate(Object state, boolean manualUpdate) { if (dataFile != null) { // Only check the file system if the file system watcher // is not enabled and the engine is using a temporary file. + logger.info("Checking for update"); logger.debug("CheckForUpdate {}", dataFile.getDataFilePath()); if (dataFile.getConfiguration().getFileSystemWatcherEnabled() == false && dataFile.getDataFilePath() != null && @@ -215,7 +217,11 @@ private AutoUpdateStatus checkForUpdate(Object state, boolean manualUpdate) { if (newDataAvailable == false && dataFile.getConfiguration().getDataUpdateUrl() != null && dataFile.getConfiguration().getDataUpdateUrl().isEmpty() == false) { - result = checkForUpdateFromUrl(dataFile); + // A failed update is tried again only when the timer is on. + result = checkForUpdateFromUrl( + dataFile, + manualUpdate == false && + dataFile.getAutomaticUpdatesEnabled()); newDataAvailable = result == AutoUpdateStatus.AUTO_UPDATE_IN_PROGRESS || result == AutoUpdateStatus.AUTO_UPDATE_SUCCESS; @@ -293,15 +299,18 @@ private AutoUpdateStatus updatedFileAvailable(AspectEngineDataFile dataFile) { return AutoUpdateStatus.AUTO_UPDATE_SUCCESS; } + logger.info("Attempting to refresh Engine '{}' with new data", + getEngineType(dataFile)); // Try to update the file multiple times to ensure the file is not locked. int tries = 0; + Exception lastFailure = null; while (tries < 10) { try { dataFile.getEngine().refreshData(dataFile.getIdentifier()); return AUTO_UPDATE_SUCCESS; } catch (Exception ex) { - logger.warn("File Update: Error applying a data update to engine '{}'", - dataFile.getEngine().getClass().getSimpleName(), ex); + lastFailure = ex; + logger.debug("Refresh attempt {} failed", tries + 1, ex); try { Thread.sleep(200); } catch (InterruptedException ignored) { @@ -309,6 +318,7 @@ private AutoUpdateStatus updatedFileAvailable(AspectEngineDataFile dataFile) { } tries++; } + logRefreshFailure(dataFile, lastFailure); return AUTO_UPDATE_REFRESH_FAILED; } @@ -380,22 +390,19 @@ public AutoUpdateStatus updateFromMemory(AspectEngineDataFile dataFile, byte[] d .writeBytes(data); } catch (Exception ex) { logger.error("An error occurred when writing to " + - "'" + dataFile.getDataFilePath() + - "'. The engine will be updated to use the new data " + - "but the file on disk will still contain old data.", - ex); + dataFile.getDataFilePath() + + ". Error detail: " + ex.getMessage(), ex); } } try { // Refresh the engine using the new data. + logger.info("Attempting to refresh Engine '{}' with new data", + getEngineType(dataFile)); dataFile.getEngine().refreshData(dataFile.getIdentifier(), data); result = AUTO_UPDATE_SUCCESS; } catch (Exception ex) { - logger.error("An error occurred when applying a " + - "data update to engine '" + dataFile.getEngine().getClass().getSimpleName() + - "'.", - ex); + logRefreshFailure(dataFile, ex); result = AutoUpdateStatus.AUTO_UPDATE_REFRESH_FAILED; } @@ -416,6 +423,7 @@ public void registerDataFile(final AspectEngineDataFile dataFile) { // We also want to do this synchronously so that execution // will block until the engine is ready. if (dataFile.getConfiguration().getUpdateOnStartup() && isFalse(alreadyRegistered)) { + logger.info("Updating on start-up"); logger.debug("Checking for Update on Startup {}", dataFile.getDataFilePath()); AutoUpdateStatus status = checkForUpdate(dataFile, false); if (ObjectUtils.notEqual(status, AUTO_UPDATE_SUCCESS) && @@ -471,11 +479,15 @@ public void run() { } } - // If file system watcher is enabled then set it up. - if (aspectDataFile.getConfiguration().getFileSystemWatcherEnabled() && + // If file system watcher is enabled then set it up. The + // automatic update switch governs the watcher as well as the + // polling timer. + if (aspectDataFile.getAutomaticUpdatesEnabled() && + aspectDataFile.getConfiguration().getFileSystemWatcherEnabled() && aspectDataFile.getConfiguration().getWatchKey() == null && aspectDataFile.getDataFilePath() != null && aspectDataFile.getDataFilePath().isEmpty() == false) { + logger.info("Creating file system watcher"); logger.debug("Initialising watch service"); final Path aspectDataFilePath = Paths.get(aspectDataFile.getDataFilePath()).toAbsolutePath(); @@ -606,32 +618,35 @@ private long applyIntervalRandomisationMillis( * If the data currently used by the engine is the newest available then * nothing will be downloaded. * - * @param dataFile the data file to use - * @param tempFile the temp file to write the data to + * @param dataFile the data file to use + * @param tempFile the temp file to write the data to + * @param automatic true if the update will be tried again if it fails * @return the {@link AutoUpdateStatus} value indicating the result */ private AutoUpdateStatus downloadFile( AspectEngineDataFile dataFile, - FileWrapper tempFile) { + FileWrapper tempFile, + boolean automatic) { String url = dataFile.getFormattedUrl(); + String engineType = getEngineType(dataFile); logger.debug("downloadFile from {}", url); try { + logger.info("Checking for update from '{}' for Engine '{}'", + url, engineType); HttpURLConnection connection = httpClient.connect(new URI(url.trim()).toURL()); if (connection == null) { - logger.error("No response from data update service at '{}' for engine '{}'", - dataFile.getFormattedUrl(), Objects.nonNull(dataFile.getEngine()) ? - dataFile.getEngine().getClass().getSimpleName() : "No Engine"); + logger.warn(connectFailure(url, engineType, automatic, + "No response from the data update service")); return AutoUpdateStatus.AUTO_UPDATE_HTTPS_ERR; } try { // Set last-modified header to ensure that a file will only // be downloaded if it is newer than the data we already have. + long ifModifiedSince = 0; if (dataFile.getConfiguration().getVerifyModifiedSince() == true) { - long ifModifiedSince = dataFile - .getDataPublishedDateTime() - .getTime(); + ifModifiedSince = getIfModifiedSince(dataFile); connection.setIfModifiedSince(ifModifiedSince); } @@ -639,7 +654,23 @@ private AutoUpdateStatus downloadFile( switch (connection.getResponseCode()) { case (HttpURLConnection.HTTP_OK): - return downloadOkResponse(connection, dataFile, tempFile); + AutoUpdateStatus downloaded; + try { + downloaded = downloadOkResponse( + connection, dataFile, tempFile, automatic); + } catch (Exception e) { + logger.warn("An error occurred while downloading " + + "a data file update for " + engineType + + " from " + url + "." + + retryNote(automatic) + + " Error detail: " + e.getMessage(), e); + return AutoUpdateStatus.AUTO_UPDATE_HTTPS_ERR; + } + if (downloaded == AutoUpdateStatus.AUTO_UPDATE_IN_PROGRESS) { + logger.info("Downloaded new data from '{}' for Engine '{}'", + url, engineType); + } + return downloaded; // Note: needed because TooManyRequests is not available // in some versions of the HttpStatusCode enum. @@ -648,8 +679,8 @@ private AutoUpdateStatus downloadFile( dataFile.getFormattedUrl(), getIdForLogging(dataFile)); return AutoUpdateStatus.AUTO_UPDATE_ERR_429_TOO_MANY_ATTEMPTS; case HttpURLConnection.HTTP_NOT_MODIFIED: - logger.debug("No data update available from '{}' for {}", - dataFile.getFormattedUrl(), getIdForLogging(dataFile)); + logger.info("No data newer than {} found at '{}' for Engine '{}'", + Instant.ofEpochMilli(ifModifiedSince), url, engineType); return AutoUpdateStatus.AUTO_UPDATE_NOT_NEEDED; case HttpURLConnection.HTTP_FORBIDDEN: logger.error("Access denied to data update service at '{}' for {}", @@ -665,11 +696,55 @@ private AutoUpdateStatus downloadFile( connection.disconnect(); } } catch (Exception e) { - logger.error("Error while processing data file download", e); + logger.warn(connectFailure(url, engineType, automatic, e.getMessage()), e); return AutoUpdateStatus.AUTO_UPDATE_HTTPS_ERR; } } + /** + * Get the time to send as If-Modified-Since. This is the publication date + * of the data file, or the last modified time of the data file on disk + * where the data file has no publication date. + */ + private long getIfModifiedSince(AspectEngineDataFile dataFile) { + Date published = dataFile.getDataPublishedDateTime(); + long publishedMillis = published == null ? 0 : published.getTime(); + if (publishedMillis > 0) { + return publishedMillis; + } + String path = dataFile.getDataFilePath(); + if (path != null && path.isEmpty() == false) { + return fileWrapperFactory.getLastModified(path); + } + return publishedMillis; + } + + private static String retryNote(boolean automatic) { + return automatic ? " Update will be attempted again later." : ""; + } + + private static String connectFailure( + String url, + String engineType, + boolean automatic, + String detail) { + return "An error occurred when connecting to " + url + + " in order to check for data file updates for " + engineType + + "." + retryNote(automatic) + " Error detail: " + detail; + } + + private void logRefreshFailure(AspectEngineDataFile dataFile, Exception ex) { + logger.error("An error occurred while applying a data file update to " + + getEngineType(dataFile) + ". Error detail: " + + (ex == null ? "none" : ex.getMessage()), ex); + } + + private String getEngineType(AspectEngineDataFile dataFile) { + return dataFile.getEngine() == null ? + "No Engine" : + dataFile.getEngine().getClass().getSimpleName(); + } + /** * Persist the body of a 200 OK download to {@code tempFile}. *

@@ -682,7 +757,8 @@ private AutoUpdateStatus downloadFile( private AutoUpdateStatus downloadOkResponse( HttpURLConnection connection, AspectEngineDataFile dataFile, - FileWrapper tempFile) throws IOException { + FileWrapper tempFile, + boolean automatic) throws IOException { logger.debug("HTTP Status is OK"); boolean verifyMd5 = dataFile.getConfiguration().getVerifyMd5(); boolean decompress = dataFile.getConfiguration().getDecompressContent(); @@ -706,6 +782,11 @@ private AutoUpdateStatus downloadOkResponse( writeBodyTo(tempFile, src); if (verifyMd5 && !md5Matches(md, connection)) { + logger.warn("An error occurred during the integrity check of new " + + "data file for " + getEngineType(dataFile) + "." + + retryNote(automatic) + + " Error detail: the MD5 hash of the download does not " + + "match the Content-MD5 header"); return AutoUpdateStatus.AUTO_UPDATE_ERR_MD5_VALIDATION_FAILED; } return AutoUpdateStatus.AUTO_UPDATE_IN_PROGRESS; @@ -782,14 +863,17 @@ private String getIdForLogging(AspectEngineDataFile dataFile) { "engine '" + dataFile.getEngine().getClass().getSimpleName() + "'"; } - private AutoUpdateStatus checkForUpdateFromUrl(AspectEngineDataFile dataFile) { + private AutoUpdateStatus checkForUpdateFromUrl( + AspectEngineDataFile dataFile, + boolean automatic) { if (dataFile.getDataFilePath() == null || dataFile.getDataFilePath().isEmpty()) { // There is no data file path specified so perform the // update entirely in memory. try { FileWrapper uncompressedData = new FileWrapperMemory(); - AutoUpdateStatus memoryDownloadResult = downloadFile(dataFile, uncompressedData); + AutoUpdateStatus memoryDownloadResult = + downloadFile(dataFile, uncompressedData, automatic); if (memoryDownloadResult != AutoUpdateStatus.AUTO_UPDATE_IN_PROGRESS) { return memoryDownloadResult; } @@ -797,6 +881,8 @@ private AutoUpdateStatus checkForUpdateFromUrl(AspectEngineDataFile dataFile) { if (dataFile.getEngine() != null) { // Tell the engine to refresh itself with // the new data. + logger.info("Attempting to refresh Engine '{}' with new data", + getEngineType(dataFile)); dataFile.getEngine().refreshData(dataFile.getIdentifier(), reader.toByteArray()); } else { @@ -807,21 +893,26 @@ private AutoUpdateStatus checkForUpdateFromUrl(AspectEngineDataFile dataFile) { } return memoryDownloadResult; } catch (Exception ex) { - logger.error("An error occurred when applying a data update to engine '{}'" , - Objects.nonNull(dataFile.getEngine())? - dataFile.getEngine().getClass().getName(): "no engine",ex); + logRefreshFailure(dataFile, ex); return AutoUpdateStatus.AUTO_UPDATE_REFRESH_FAILED; } } - + if (dataFile.getTempDataDirPath() == null || + dataFile.getTempDataDirPath().isEmpty()) { + logger.error("The data file '" + dataFile.getIdentifier() + + "' is checking for updates but does not have a temporary " + + "file path configured."); + return AutoUpdateStatus.AUTO_UPDATE_REFRESH_FAILED; + } Path uncompressedTempFile = Paths.get( dataFile.getTempDataDirPath(), dataFile.getIdentifier() + "-" + randomUUID() + ".tmp"); FileWrapper uncompressedData = fileWrapperFactory.build(uncompressedTempFile.toString()); try { // Check if there is an update and download it if there is - AutoUpdateStatus result = downloadFile(dataFile, uncompressedData); + AutoUpdateStatus result = + downloadFile(dataFile, uncompressedData, automatic); if (result == AutoUpdateStatus.AUTO_UPDATE_IN_PROGRESS) { WatchKey watchKey = dataFile.getConfiguration().getWatchKey(); @@ -847,12 +938,9 @@ private AutoUpdateStatus checkForUpdateFromUrl(AspectEngineDataFile dataFile) { logger.debug("Reading file {}", d.read()); } } catch (Exception ex) { - logger.error("An error occurred when moving a data file to replace " + - "the existing one at {} for engine '{}'.", - dataFile.getDataFilePath(), - Objects.nonNull(dataFile.getEngine()) ? - dataFile.getEngine().getClass().getSimpleName() : "No engine defined", - ex); + logger.error("An error occurred when copying a data file to " + + "replace the existing one at '{}'.", + dataFile.getDataFilePath(), ex); result = AutoUpdateStatus.AUTO_UPDATE_NEW_FILE_CANT_RENAME; } // Make sure to enable the file watcher again if needed. From 733a1e2d3314d03658b95888e1df320520878710 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 18:44:27 +0000 Subject: [PATCH 3/6] Test that HTTP status failures are warnings --- .../DataUpdateServiceLoggingTests.java | 28 +++++++++++++++++++ .../services/DataUpdateServiceTests.java | 4 +-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceLoggingTests.java b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceLoggingTests.java index dcdd6b569..d2f7ecbd7 100644 --- a/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceLoggingTests.java +++ b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceLoggingTests.java @@ -304,6 +304,34 @@ public void ConnectFails_AutoUpdateOff_DoesNotPromiseAnotherTry() engineType + ". Error detail: refused"); } + private void assertStatusWarned(int status, String detail) + throws Exception { + when(connection.getResponseCode()).thenReturn(status); + newDataFile(true, false, false); + + dataUpdate.checkForUpdate(engine); + + assertLogged(Level.WARN, "An error occurred while downloading a data " + + "file update for " + engineType + " from " + UPDATE_URL + ". " + + "Error detail: " + detail); + assertTrue(logged(Level.ERROR).isEmpty()); + } + + @Test + public void TooManyRequests_WarnsAboutTheDownload() throws Exception { + assertStatusWarned(429, "HTTP status code 429 (too many requests)"); + } + + @Test + public void Forbidden_WarnsAboutTheDownload() throws Exception { + assertStatusWarned(403, "HTTP status code 403 (access denied)"); + } + + @Test + public void OtherStatus_WarnsAboutTheDownload() throws Exception { + assertStatusWarned(500, "HTTP status code 500"); + } + @Test public void Md5DoesNotMatch_WarnsAboutIntegrityCheck() throws Exception { when(connection.getResponseCode()) diff --git a/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceTests.java b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceTests.java index 2106f669e..2d7c2263c 100644 --- a/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceTests.java +++ b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceTests.java @@ -1398,8 +1398,8 @@ public void call(Object sender, DataUpdateService.DataUpdateCompleteArgs args) { assertTrue("The 'checkForUpdateComplete' " + "event was never fired", completed); - // Ignore the error that is logged due to the 429 - ignoreErrors = 1; + // Ignore the warning that is logged due to the 429 + ignoreWranings = 1; // Check that the timer has been set to go off again. assertNotNull(file.getFuture()); // Check that the timer has been set to expire in 10 seconds. From 98f3a9a1d772b400a8c70abd1ba71cae9948e01b Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 18:44:31 +0000 Subject: [PATCH 4/6] Log a 429, a 403 and any other failing HTTP status as a warning The Logging section of the data update specification makes a failed download a warning, with the error detail from the server. --- .../services/DataUpdateServiceDefault.java | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/DataUpdateServiceDefault.java b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/DataUpdateServiceDefault.java index 5eb514cbd..15712e240 100644 --- a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/DataUpdateServiceDefault.java +++ b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/DataUpdateServiceDefault.java @@ -659,11 +659,8 @@ private AutoUpdateStatus downloadFile( downloaded = downloadOkResponse( connection, dataFile, tempFile, automatic); } catch (Exception e) { - logger.warn("An error occurred while downloading " + - "a data file update for " + engineType + - " from " + url + "." + - retryNote(automatic) + - " Error detail: " + e.getMessage(), e); + logger.warn(downloadFailure(url, engineType, + automatic, e.getMessage()), e); return AutoUpdateStatus.AUTO_UPDATE_HTTPS_ERR; } if (downloaded == AutoUpdateStatus.AUTO_UPDATE_IN_PROGRESS) { @@ -675,21 +672,20 @@ private AutoUpdateStatus downloadFile( // Note: needed because TooManyRequests is not available // in some versions of the HttpStatusCode enum. case (429): - logger.error("Too many requests to '{}' for {}", - dataFile.getFormattedUrl(), getIdForLogging(dataFile)); + logger.warn(downloadFailure(url, engineType, automatic, + "HTTP status code 429 (too many requests)")); return AutoUpdateStatus.AUTO_UPDATE_ERR_429_TOO_MANY_ATTEMPTS; case HttpURLConnection.HTTP_NOT_MODIFIED: logger.info("No data newer than {} found at '{}' for Engine '{}'", Instant.ofEpochMilli(ifModifiedSince), url, engineType); return AutoUpdateStatus.AUTO_UPDATE_NOT_NEEDED; case HttpURLConnection.HTTP_FORBIDDEN: - logger.error("Access denied to data update service at '{}' for {}", - dataFile.getFormattedUrl(), getIdForLogging(dataFile)); + logger.warn(downloadFailure(url, engineType, automatic, + "HTTP status code 403 (access denied)")); return AutoUpdateStatus.AUTO_UPDATE_ERR_403_FORBIDDEN; default: - logger.error("HTTP status code '{}' from data update service at '{}' for {}", - connection.getResponseCode(), dataFile.getFormattedUrl(), - getIdForLogging(dataFile)); + logger.warn(downloadFailure(url, engineType, automatic, + "HTTP status code " + connection.getResponseCode())); return AutoUpdateStatus.AUTO_UPDATE_HTTPS_ERR; } } finally { @@ -733,6 +729,16 @@ private static String connectFailure( "." + retryNote(automatic) + " Error detail: " + detail; } + private static String downloadFailure( + String url, + String engineType, + boolean automatic, + String detail) { + return "An error occurred while downloading a data file update for " + + engineType + " from " + url + "." + retryNote(automatic) + + " Error detail: " + detail; + } + private void logRefreshFailure(AspectEngineDataFile dataFile, Exception ex) { logger.error("An error occurred while applying a data file update to " + getEngineType(dataFile) + ". Error detail: " + From 616953189407ac8d65c4214070e9f2c4745dda49 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 23:26:14 +0000 Subject: [PATCH 5/6] Test that the update on start-up log names the engine type --- ...OnPremiseAspectEngineBuilderBaseTests.java | 24 +++++++++++++++++++ .../DataUpdateServiceLoggingTests.java | 15 ++++++++++++ 2 files changed, 39 insertions(+) diff --git a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/FiftyOneDegreesOnPremiseAspectEngineBuilderBaseTests.java b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/FiftyOneDegreesOnPremiseAspectEngineBuilderBaseTests.java index 9caab246c..bb7231527 100644 --- a/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/FiftyOneDegreesOnPremiseAspectEngineBuilderBaseTests.java +++ b/pipeline/pipeline.engines.fiftyone/src/test/java/fiftyone/pipeline/engines/fiftyone/flowelements/FiftyOneDegreesOnPremiseAspectEngineBuilderBaseTests.java @@ -26,9 +26,11 @@ import fiftyone.pipeline.engines.Constants; import fiftyone.pipeline.engines.data.AspectData; import fiftyone.pipeline.engines.data.AspectEngineDataFile; +import fiftyone.pipeline.engines.data.AspectEngineDataFileDefault; import fiftyone.pipeline.engines.data.AspectPropertyMetaData; import fiftyone.pipeline.engines.fiftyone.data.FiftyOneDataFile; import fiftyone.pipeline.engines.services.DataUpdateService; +import org.mockito.ArgumentCaptor; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.invocation.InvocationOnMock; @@ -174,6 +176,28 @@ public void UpdateOnStartup_AutoUpdateOff_DataFileRegistered() verify(updateService, times(1)).registerDataFile(any()); } + /** + * The engine does not exist when the data file is registered, so the + * data file carries the engine's type name for the update service's log. + */ + @Test + public void UpdateOnStartup_DataFileCarriesTheEngineTypeName() + throws Exception { + ImplementedBuilder builder = newBuilder() + .setAutoUpdate(false) + .setDataUpdateOnStartup(true) + .setDataUpdateLicenseKey(licenseKey); + + builder.build("", false); + + ArgumentCaptor registered = + ArgumentCaptor.forClass(AspectEngineDataFile.class); + verify(updateService).registerDataFile(registered.capture()); + assertEquals("Implemented", + ((AspectEngineDataFileDefault) registered.getValue()) + .getEngineTypeName()); + } + @Test public void AutoUpdateAndUpdateOnStartupOff_DataFileNotRegistered() throws Exception { diff --git a/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceLoggingTests.java b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceLoggingTests.java index d2f7ecbd7..31e4a0f1c 100644 --- a/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceLoggingTests.java +++ b/pipeline/pipeline.engines/src/test/java/fiftyone/pipeline/engines/services/DataUpdateServiceLoggingTests.java @@ -240,6 +240,21 @@ public void UpdateOnStartup_LogsUpdatingOnStartUp() throws Exception { assertLogged(Level.INFO, "Updating on start-up"); } + @Test + public void UpdateOnStartup_EngineNotBuiltYet_LogsTheEngineTypeName() + throws Exception { + when(connection.getResponseCode()) + .thenReturn(HttpURLConnection.HTTP_NOT_MODIFIED); + AspectEngineDataFileDefault file = newDataFile(false, false, true); + file.setEngine(null); + file.setEngineTypeName("ExampleEngine"); + + dataUpdate.registerDataFile(file); + + assertLogged(Level.INFO, "Checking for update from '" + UPDATE_URL + + "' for Engine 'ExampleEngine'"); + } + @Test public void UpdateOnStartup_AutoUpdateOff_StartsNoTimer() throws Exception { when(connection.getResponseCode()) From adce29b88e1c4b76eb65c1b8f573f050104b2c7e Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Mon, 9 Nov 2026 23:26:27 +0000 Subject: [PATCH 6/6] Name the engine in the update on start-up log before the engine exists The builder gives the data file its engine's type name, taken from the builder's own name, and the update service logs it until the engine is set. --- .../data/AspectEngineDataFileDefault.java | 18 ++++++++++++++++++ .../OnPremiseAspectEngineBuilderBase.java | 9 +++++++++ .../services/DataUpdateServiceDefault.java | 12 +++++++++--- 3 files changed, 36 insertions(+), 3 deletions(-) diff --git a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/data/AspectEngineDataFileDefault.java b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/data/AspectEngineDataFileDefault.java index 100d23207..55ecaa049 100644 --- a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/data/AspectEngineDataFileDefault.java +++ b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/data/AspectEngineDataFileDefault.java @@ -41,6 +41,7 @@ public class AspectEngineDataFileDefault implements AspectEngineDataFile { private DataFileConfiguration configuration; private DataUpdateService dataUpdateService = null; private String tempDataDirPath; + private String engineTypeName; @Override public String getIdentifier() { @@ -87,6 +88,23 @@ public void setTempDataFilePath(String path) { this.tempDataFilePath = path; } + /** + * The type name of the engine this data file is for, which the update + * service logs until the engine is set. + * @return the engine type name, or null if it is not known + */ + public String getEngineTypeName() { + return engineTypeName; + } + + /** + * Set the type name of the engine this data file is for. + * @param engineTypeName the engine type name + */ + public void setEngineTypeName(String engineTypeName) { + this.engineTypeName = engineTypeName; + } + @Override public String getTempDataDirPath() { if (tempDataDirPath == null && diff --git a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/flowelements/OnPremiseAspectEngineBuilderBase.java b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/flowelements/OnPremiseAspectEngineBuilderBase.java index c5a410a29..822fb743f 100644 --- a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/flowelements/OnPremiseAspectEngineBuilderBase.java +++ b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/flowelements/OnPremiseAspectEngineBuilderBase.java @@ -187,6 +187,15 @@ protected void preCreateEngine() { dataFile.setIdentifier(dataFileConfig.getIdentifier()); dataFile.setConfiguration(dataFileConfig); dataFile.setTempDataDirPath(tempDir); + if (dataFile instanceof AspectEngineDataFileDefault) { + // By convention an engine is named for its builder. + String builderName = getClass().getSimpleName(); + ((AspectEngineDataFileDefault) dataFile).setEngineTypeName( + builderName.endsWith("Builder") ? + builderName.substring( + 0, builderName.length() - "Builder".length()) : + builderName); + } boolean autoUpdate = dataFileConfig.getAutomaticUpdatesEnabled(); boolean updateOnStartup = dataFileConfig.getUpdateOnStartup(); diff --git a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/DataUpdateServiceDefault.java b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/DataUpdateServiceDefault.java index 15712e240..d25a77b91 100644 --- a/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/DataUpdateServiceDefault.java +++ b/pipeline/pipeline.engines/src/main/java/fiftyone/pipeline/engines/services/DataUpdateServiceDefault.java @@ -746,9 +746,15 @@ private void logRefreshFailure(AspectEngineDataFile dataFile, Exception ex) { } private String getEngineType(AspectEngineDataFile dataFile) { - return dataFile.getEngine() == null ? - "No Engine" : - dataFile.getEngine().getClass().getSimpleName(); + if (dataFile.getEngine() != null) { + return dataFile.getEngine().getClass().getSimpleName(); + } + // The engine is built after update on start-up has run. + if (dataFile instanceof AspectEngineDataFileDefault && + ((AspectEngineDataFileDefault) dataFile).getEngineTypeName() != null) { + return ((AspectEngineDataFileDefault) dataFile).getEngineTypeName(); + } + return "No Engine"; } /**