From 1bcd7d164700f7ac20d65e4654f9ae37757dfee3 Mon Sep 17 00:00:00 2001 From: Brian Flores Date: Wed, 30 Oct 2024 12:06:22 -0700 Subject: [PATCH] fix: better exception message Previously used docId for saying where the problem is, there is no gauarantee that this field exists instead _id is always guaranteed to be there. Also logging of the mapping was provided because it may be misleading when a user has a number as a string. It can be confusing for a user to something like [3.3] is not a number when the reality is that it was sent like "3.3" thus the type of the value is provided to help users understand better Signed-off-by: Brian Flores --- .../rerank/ByFieldRerankProcessor.java | 27 ++++++++++++++----- .../rerank/ByFieldRerankProcessorTests.java | 11 +++++--- 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/opensearch/neuralsearch/processor/rerank/ByFieldRerankProcessor.java b/src/main/java/org/opensearch/neuralsearch/processor/rerank/ByFieldRerankProcessor.java index 28bf7866f..27987a8c6 100644 --- a/src/main/java/org/opensearch/neuralsearch/processor/rerank/ByFieldRerankProcessor.java +++ b/src/main/java/org/opensearch/neuralsearch/processor/rerank/ByFieldRerankProcessor.java @@ -162,26 +162,41 @@ public void rescoreSearchResponse( */ public void byFieldSearchHitValidator(final SearchHit hit) { if (!hit.hasSource()) { - log.error(String.format(Locale.ROOT, "There is no source field to be able to perform rerank on hit [%d]", hit.docId())); + log.error(String.format(Locale.ROOT, "There is no source field to be able to perform rerank on hit [%s]", hit.getId())); throw new IllegalArgumentException( - String.format(Locale.ROOT, "There is no source field to be able to perform rerank on hit [%d]", hit.docId()) + String.format(Locale.ROOT, "There is no source field to be able to perform rerank on hit [%s]", hit.getId()) ); } Map sourceMap = hit.getSourceAsMap(); if (!mappingExistsInSource(sourceMap, targetField)) { - log.error(String.format(Locale.ROOT, "The field to rerank [%s] is not found at hit [%d]", targetField, hit.docId())); + log.error(String.format(Locale.ROOT, "The field to rerank [%s] is not found at hit [%s]", targetField, hit.getId())); - throw new IllegalArgumentException(String.format(Locale.ROOT, "The field to rerank by is not found at hit [%d]", hit.docId())); + throw new IllegalArgumentException(String.format(Locale.ROOT, "The field to rerank by is not found at hit [%s]", hit.getId())); } Optional val = getValueFromSource(sourceMap, targetField); if (!(val.get() instanceof Number)) { - log.error(String.format(Locale.ROOT, "The field mapping to rerank [%s: %s] is not Numerical", targetField, val.orElse(null))); + // Strictly get the type of value removing the prefix of getClass() having a value is guaranteed so no NPE check + String typeOfMapping = val.get().getClass().toString().replace("class ", ""); + log.error( + String.format( + Locale.ROOT, + "The field mapping to rerank [%s: %s] is not Numerical, instead of type [%s]", + targetField, + val.orElse(null), + typeOfMapping + ) + ); throw new IllegalArgumentException( - String.format(Locale.ROOT, "The field mapping to rerank by [%s] is not Numerical", val.orElse(null)) + String.format( + Locale.ROOT, + "The field mapping to rerank by [%s] is not Numerical, instead of type [%s]", + val.orElse(null), + typeOfMapping + ) ); } diff --git a/src/test/java/org/opensearch/neuralsearch/processor/rerank/ByFieldRerankProcessorTests.java b/src/test/java/org/opensearch/neuralsearch/processor/rerank/ByFieldRerankProcessorTests.java index a2555663d..6f32ed036 100644 --- a/src/test/java/org/opensearch/neuralsearch/processor/rerank/ByFieldRerankProcessorTests.java +++ b/src/test/java/org/opensearch/neuralsearch/processor/rerank/ByFieldRerankProcessorTests.java @@ -869,7 +869,7 @@ public void testRerank_throwsExceptionOnNoSource_WhenSearchResponseHasNoSourceMa ArgumentCaptor argumentCaptor = ArgumentCaptor.forClass(Exception.class); verify(listener, times(1)).onFailure(argumentCaptor.capture()); - assertEquals("There is no source field to be able to perform rerank on hit [" + 1 + "]", argumentCaptor.getValue().getMessage()); + assertEquals("There is no source field to be able to perform rerank on hit [" + 2 + "]", argumentCaptor.getValue().getMessage()); assert (argumentCaptor.getValue() instanceof IllegalArgumentException); } @@ -907,7 +907,7 @@ public void testRerank_throwsExceptionOnMappingNotExistingInSource_WhenSearchRes ArgumentCaptor argumentCaptor = ArgumentCaptor.forClass(Exception.class); verify(listener, times(1)).onFailure(argumentCaptor.capture()); - assertEquals("The field to rerank by is not found at hit [" + 1 + "]", argumentCaptor.getValue().getMessage()); + assertEquals("The field to rerank by is not found at hit [" + 2 + "]", argumentCaptor.getValue().getMessage()); assert (argumentCaptor.getValue() instanceof IllegalArgumentException); } @@ -947,7 +947,7 @@ public void testRerank_throwsExceptionOnHavingEmptyMapping_WhenTargetFieldHasNul ArgumentCaptor argumentCaptor = ArgumentCaptor.forClass(Exception.class); verify(listener, times(1)).onFailure(argumentCaptor.capture()); - assertEquals("The field to rerank by is not found at hit [" + 1 + "]", argumentCaptor.getValue().getMessage()); + assertEquals("The field to rerank by is not found at hit [" + 2 + "]", argumentCaptor.getValue().getMessage()); assert (argumentCaptor.getValue() instanceof IllegalArgumentException); } @@ -985,7 +985,10 @@ public void testRerank_throwsExceptionOnHavingNonNumericValue_WhenTargetFieldHas ArgumentCaptor argumentCaptor = ArgumentCaptor.forClass(Exception.class); verify(listener, times(1)).onFailure(argumentCaptor.capture()); - assertEquals("The field mapping to rerank by [hello world] is not Numerical", argumentCaptor.getValue().getMessage()); + assertEquals( + "The field mapping to rerank by [hello world] is not Numerical, instead of type [java.lang.String]", + argumentCaptor.getValue().getMessage() + ); assert (argumentCaptor.getValue() instanceof IllegalArgumentException); }