Merge pull request #2299 from ControlSystemStudio/CSSTUIDO-1703

Better error message on Olog search failure

Author: shroffk

app/logbook/olog/client-es/src/main/java/org/phoebus/olog/es/api/OlogClient.java

@@ -46,7 +46,6 @@
 import java.util.Map;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import java.util.stream.Collectors;
 
 /**
  * A client to the Olog-es webservice
@@ -68,20 +67,20 @@ public class OlogClient implements LogClient {
      */
     public static class OlogClientBuilder {
         // required
-        private URI ologURI = null;
+        private final URI ologURI;
 
         // optional
         private boolean withHTTPAuthentication = true;
 
         private ClientConfig clientConfig = null;
         @SuppressWarnings("unused")
         private SSLContext sslContext = null;
-        private String protocol = null;
+        private final String protocol;
         private String username = null;
         private String password = null;
         private String connectTimeoutAsString = null;
 
-        private OlogProperties properties = new OlogProperties();
+        private final OlogProperties properties = new OlogProperties();
 
         private OlogClientBuilder() {
             this.ologURI = URI.create(this.properties.getPreferenceValue("olog_url"));
@@ -92,7 +91,7 @@ private OlogClientBuilder() {
          * Creates a {@link OlogClientBuilder} for a CF client to Default URL in the
          * channelfinder.properties.
          *
-         * @return
+         * @return The builder
          */
         public static OlogClientBuilder serviceURL() {
             return new OlogClientBuilder();
@@ -101,7 +100,7 @@ public static OlogClientBuilder serviceURL() {
         /**
          * Enable of Disable the HTTP authentication on the client connection.
          *
-         * @param withHTTPAuthentication
+         * @param withHTTPAuthentication Whether to use authentication or not
          * @return {@link OlogClientBuilder}
          */
         public OlogClientBuilder withHTTPAuthentication(boolean withHTTPAuthentication) {
@@ -112,7 +111,7 @@ public OlogClientBuilder withHTTPAuthentication(boolean withHTTPAuthentication)
         /**
          * Set the username to be used for HTTP Authentication.
          *
-         * @param username
+         * @param username User's  identity
          * @return {@link OlogClientBuilder}
          */
         public OlogClientBuilder username(String username) {
@@ -123,7 +122,7 @@ public OlogClientBuilder username(String username) {
         /**
          * Set the password to be used for the HTTP Authentication.
          *
-         * @param password
+         * @param password User's password
          * @return {@link OlogClientBuilder}
          */
         public OlogClientBuilder password(String password) {
@@ -149,7 +148,7 @@ public OlogClient create() {
             this.username = ifNullReturnPreferenceValue(this.username, "username");
             this.password = ifNullReturnPreferenceValue(this.password, "password");
             this.connectTimeoutAsString = ifNullReturnPreferenceValue(this.connectTimeoutAsString, "connectTimeout");
-            Integer connectTimeout = 0;
+            int connectTimeout = 0;
             try {
                 connectTimeout = Integer.parseInt(connectTimeoutAsString);
             } catch (NumberFormatException e) {
@@ -216,7 +215,7 @@ private LogEntry save(LogEntry log, LogEntry inReplyTo) throws LogbookException
 
             if (clientResponse.getStatus() < 300) {
                 OlogLog createdLog = OlogObjectMappers.logEntryDeserializer.readValue(clientResponse.getEntityInputStream(), OlogLog.class);
-                log.getAttachments().stream().forEach(attachment -> {
+                log.getAttachments().forEach(attachment -> {
                     FormDataMultiPart form = new FormDataMultiPart();
                     // Add id only if it is set, otherwise Jersey will complain and cause the submission to fail.
                     if (attachment.getId() != null && !attachment.getId().isEmpty()) {
@@ -277,12 +276,11 @@ public LogEntry getLog(Long logId) {
     @Override
     public LogEntry findLogById(Long logId) {
         try {
-            OlogLog ologLog = OlogObjectMappers.logEntryDeserializer.readValue(
+            return OlogObjectMappers.logEntryDeserializer.readValue(
                     service
                             .path("logs")
                             .path(logId.toString())
                             .accept(MediaType.APPLICATION_JSON).get(String.class), OlogLog.class);
-            return ologLog;
         } catch (JsonProcessingException e) {
             return null;
         }
@@ -300,7 +298,7 @@ public List<LogEntry> findLogs(Map<String, String> map) throws RuntimeException
      *
      * @param searchParams Map of search parameters/expressions
      * @return A list of matching {@link LogEntry}s
-     * @throws RuntimeException
+     * @throws RuntimeException If search fails, e.g. due to invalid search parameters
      */
     private SearchResult findLogs(MultivaluedMap<String, String> searchParams) throws RuntimeException {
         try {
@@ -311,10 +309,15 @@ private SearchResult findLogs(MultivaluedMap<String, String> searchParams) throw
                             .accept(MediaType.APPLICATION_JSON)
                             .get(String.class),
                     OlogSearchResult.class);
-            return SearchResult.of(ologSearchResult.getLogs().stream().collect(Collectors.toList()),
+            return SearchResult.of(new ArrayList<>(ologSearchResult.getLogs()),
                                    ologSearchResult.getHitCount());
         } catch (UniformInterfaceException | ClientHandlerException | IOException e) {
             logger.log(Level.WARNING, "failed to retrieve log entries", e);
+            if(e instanceof UniformInterfaceException){
+                if(((UniformInterfaceException) e).getResponse().getStatus() == Status.BAD_REQUEST.getStatusCode()){
+                    throw new RuntimeException(Messages.BadRequestFailure);
+                }
+            }
             throw new RuntimeException(e);
         }
     }
@@ -373,8 +376,7 @@ public Collection<String> listAttributes(String propertyName) {
 
     @Override
     public List<LogEntry> listLogs() {
-        List<LogEntry> logEntries = new ArrayList<>();
-        return logEntries;
+        return new ArrayList<>();
     }
 
     /**
@@ -400,11 +402,10 @@ public Collection<String> listLevels() {
     @Override
     public Collection<Logbook> listLogbooks() {
         try {
-            List<Logbook> logbooks = OlogObjectMappers.logEntryDeserializer.readValue(
+            return OlogObjectMappers.logEntryDeserializer.readValue(
                     service.path("logbooks").accept(MediaType.APPLICATION_JSON).get(String.class),
                     new TypeReference<List<Logbook>>() {
                     });
-            return logbooks;
         } catch (UniformInterfaceException | ClientHandlerException | IOException e) {
             logger.log(Level.WARNING, "Unable to get logbooks from service", e);
             return Collections.emptySet();
@@ -414,11 +415,10 @@ public Collection<Logbook> listLogbooks() {
     @Override
     public Collection<Property> listProperties() {
         try {
-            List<Property> properties = OlogObjectMappers.logEntryDeserializer.readValue(
+            return OlogObjectMappers.logEntryDeserializer.readValue(
                     service.path("properties").accept(MediaType.APPLICATION_JSON).get(String.class),
                     new TypeReference<List<Property>>() {
                     });
-            return properties;
         } catch (UniformInterfaceException | ClientHandlerException | IOException e) {
             logger.log(Level.WARNING, "failed to list olog properties", e);
             return Collections.emptySet();
@@ -428,11 +428,10 @@ public Collection<Property> listProperties() {
     @Override
     public Collection<Tag> listTags() {
         try {
-            List<Tag> tags = OlogObjectMappers.logEntryDeserializer.readValue(
+            return OlogObjectMappers.logEntryDeserializer.readValue(
                     service.path("tags").accept(MediaType.APPLICATION_JSON).get(String.class),
                     new TypeReference<List<Tag>>() {
                     });
-            return tags;
         } catch (UniformInterfaceException | ClientHandlerException | IOException e) {
             logger.log(Level.WARNING, "failed to retrieve olog tags", e);
             return Collections.emptySet();
@@ -449,7 +448,7 @@ public String getServiceUrl() {
     }
 
     @Override
-    public LogEntry updateLogEntry(LogEntry logEntry) throws LogbookException {
+    public LogEntry updateLogEntry(LogEntry logEntry) {
         ClientResponse clientResponse;
 
         try {
@@ -469,9 +468,7 @@ public LogEntry updateLogEntry(LogEntry logEntry) throws LogbookException {
     @Override
     public SearchResult search(Map<String, String> map) {
         MultivaluedMap<String, String> mMap = new MultivaluedMapImpl();
-        map.forEach((k, v) -> {
-            mMap.putSingle(k, v);
-        });
+        map.forEach(mMap::putSingle);
         return findLogs(mMap);
     }
 

app/logbook/olog/ui/src/main/java/org/phoebus/logbook/olog/ui/LogEntryTableViewController.java

@@ -8,7 +8,6 @@
 import javafx.beans.property.SimpleBooleanProperty;
 import javafx.beans.property.SimpleIntegerProperty;
 import javafx.beans.property.SimpleObjectProperty;
-import javafx.beans.value.ChangeListener;
 import javafx.collections.FXCollections;
 import javafx.collections.ObservableList;
 import javafx.css.PseudoClass;
@@ -31,7 +30,6 @@
 import javafx.scene.control.TableColumn;
 import javafx.scene.control.TableView;
 import javafx.scene.control.TextField;
-import javafx.scene.image.ImageView;
 import javafx.scene.input.KeyCode;
 import javafx.scene.input.KeyCodeCombination;
 import javafx.scene.input.KeyCombination;
@@ -51,7 +49,6 @@
 import org.phoebus.olog.es.api.model.LogGroupProperty;
 import org.phoebus.ui.dialog.DialogHelper;
 import org.phoebus.ui.dialog.ExceptionDetailsErrorDialog;
-import org.phoebus.ui.javafx.ImageCache;
 
 import java.io.IOException;
 import java.util.List;
@@ -107,7 +104,7 @@ public class LogEntryTableViewController extends LogbookSearchController {
     private final ObservableList<LogEntry> selectedLogEntries = FXCollections.observableArrayList();
     private final Logger logger = Logger.getLogger(LogEntryTableViewController.class.getName());
 
-    private SimpleBooleanProperty showDetails = new SimpleBooleanProperty();
+    private final SimpleBooleanProperty showDetails = new SimpleBooleanProperty();
 
     /**
      * Constructor.
@@ -129,13 +126,7 @@ public LogEntryTableViewController(LogClient logClient, OlogQueryManager ologQue
     private final OlogQueryManager ologQueryManager;
     private final ObservableList<OlogQuery> ologQueries = FXCollections.observableArrayList();
 
-    /**
-     * The listener called when user selects an item in the {@link ComboBox}
-     * drop-down, or when a value is set implicitly from code when updating the list of items in the drop-down.
-     */
-    private ChangeListener<OlogQuery> onActionListener;
-
-    private SearchParameters searchParameters;
+    private final SearchParameters searchParameters;
 
     private LogEntry selectedLogEntry;
 
@@ -144,16 +135,12 @@ public void initialize() {
         configureComboBox();
         ologQueries.setAll(ologQueryManager.getQueries());
 
-        searchParameters.addListener((observable, oldValue, newValue) -> {
-            query.getEditor().setText(newValue);
-        });
+        searchParameters.addListener((observable, oldValue, newValue) -> query.getEditor().setText(newValue));
 
         tableView.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
 
         MenuItem groupSelectedEntries = new MenuItem(Messages.GroupSelectedEntries);
-        groupSelectedEntries.setOnAction(e -> {
-            createLogEntryGroup();
-        });
+        groupSelectedEntries.setOnAction(e -> createLogEntryGroup());
 
         groupSelectedEntries.disableProperty()
                 .bind(Bindings.createBooleanBinding(() ->
@@ -190,37 +177,35 @@ public void initialize() {
         descriptionCol = new TableColumn<>();
         descriptionCol.setMaxWidth(1f * Integer.MAX_VALUE * 100);
         descriptionCol.setCellValueFactory(col -> new SimpleObjectProperty(col.getValue()));
-        descriptionCol.setCellFactory(col -> {
-            return new TableCell<>() {
-                private final Node graphic;
-                private final PseudoClass childlessTopLevel =
-                        PseudoClass.getPseudoClass("grouped");
-                private final LogEntryCellController controller;
-
-                {
-                    try {
-                        FXMLLoader loader = new FXMLLoader(getClass().getResource("LogEntryCell.fxml"));
-                        graphic = loader.load();
-                        controller = loader.getController();
-                    } catch (IOException exc) {
-                        throw new RuntimeException(exc);
-                    }
+        descriptionCol.setCellFactory(col -> new TableCell<>() {
+            private final Node graphic;
+            private final PseudoClass childlessTopLevel =
+                    PseudoClass.getPseudoClass("grouped");
+            private final LogEntryCellController controller;
+
+            {
+                try {
+                    FXMLLoader loader = new FXMLLoader(getClass().getResource("LogEntryCell.fxml"));
+                    graphic = loader.load();
+                    controller = loader.getController();
+                } catch (IOException exc) {
+                    throw new RuntimeException(exc);
                 }
+            }
 
-                @Override
-                public void updateItem(TableViewListItem logEntry, boolean empty) {
-                    super.updateItem(logEntry, empty);
-                    if (empty) {
-                        setGraphic(null);
-                        pseudoClassStateChanged(childlessTopLevel, false);
-                    } else {
-                        controller.setLogEntry(logEntry);
-                        setGraphic(graphic);
-                        boolean b = LogGroupProperty.getLogGroupProperty(logEntry.getLogEntry()).isPresent();
-                        pseudoClassStateChanged(childlessTopLevel, b);
-                    }
+            @Override
+            public void updateItem(TableViewListItem logEntry, boolean empty) {
+                super.updateItem(logEntry, empty);
+                if (empty) {
+                    setGraphic(null);
+                    pseudoClassStateChanged(childlessTopLevel, false);
+                } else {
+                    controller.setLogEntry(logEntry);
+                    setGraphic(graphic);
+                    boolean b = LogGroupProperty.getLogGroupProperty(logEntry.getLogEntry()).isPresent();
+                    pseudoClassStateChanged(childlessTopLevel, b);
                 }
-            };
+            }
         });
 
         tableView.getColumns().add(descriptionCol);
@@ -230,9 +215,7 @@ public void updateItem(TableViewListItem logEntry, boolean empty) {
 
         searchResultView.disableProperty().bind(searchInProgress);
 
-        pagination.currentPageIndexProperty().addListener((a, b, c) -> {
-            search();
-        });
+        pagination.currentPageIndexProperty().addListener((a, b, c) -> search());
 
         pageSizeTextField.setText(Integer.toString(pageSizeProperty.get()));
 
@@ -339,7 +322,7 @@ public void search() {
                     searchInProgress.set(false);
                     setSearchResult(searchResult1);
                     logger.log(Level.INFO, "Starting periodic search: " + queryString);
-                    periodicSearch(params, searchResult -> setSearchResult(searchResult));
+                    periodicSearch(params, this::setSearchResult);
                     List<OlogQuery> queries = ologQueryManager.getQueries();
                     Platform.runLater(() -> {
                         ologQueries.setAll(queries);
@@ -348,7 +331,7 @@ public void search() {
                 },
                 (msg, ex) -> {
                     searchInProgress.set(false);
-                    ExceptionDetailsErrorDialog.openError("Logbook Search Error", ex.getMessage(), ex);
+                    ExceptionDetailsErrorDialog.openError(Messages.LogbooksSearchFailTitle, ex.getMessage(), null);
                 });
     }
 
@@ -433,7 +416,7 @@ private void configureComboBox() {
                 new Callback<>() {
                     @Override
                     public ListCell<OlogQuery> call(ListView<OlogQuery> param) {
-                        final ListCell<OlogQuery> cell = new ListCell<>() {
+                        return new ListCell<>() {
                             @Override
                             public void updateItem(OlogQuery item,
                                                    boolean empty) {
@@ -450,7 +433,6 @@ public void updateItem(OlogQuery item,
                                 }
                             }
                         };
-                        return cell;
                     }
                 });
 
@@ -479,8 +461,8 @@ public OlogQuery fromString(String s) {
      * log entry meta data should be rendered in the list view.
      */
     public static class TableViewListItem {
-        private SimpleBooleanProperty showDetails = new SimpleBooleanProperty(true);
-        private LogEntry logEntry;
+        private final SimpleBooleanProperty showDetails = new SimpleBooleanProperty(true);
+        private final LogEntry logEntry;
 
         public TableViewListItem(LogEntry logEntry, boolean showDetails) {
             this.logEntry = logEntry;

app/logbook/olog/ui/src/main/java/org/phoebus/logbook/olog/ui/Messages.java

@@ -27,6 +27,7 @@ public class Messages
                          GroupSelectedEntries,
                          Level,
                          Logbooks,
+                         LogbooksSearchFailTitle,
                          LogbookServiceUnavailableTitle,
                          LogbookServiceHasNoLogbooks,
                          LogbooksTitle,

app/logbook/olog/ui/src/main/resources/org/phoebus/logbook/olog/ui/messages.properties

@@ -42,6 +42,7 @@ Level=Level
 Logbooks=Logbooks:
 LogbookServiceUnavailableTitle=Cannot create logbook entry
 LogbookServiceHasNoLogbooks=Logbook service "{0}" has no logbooks or is not available.
+LogbooksSearchFailTitle=Logbook Search Error
 LogbooksTitle=Select Logbooks
 LogbooksTooltip=Add logbook to the log entry.
 NoClipboardContent=Clipboard does not contain any content that may be added as attachment.