Suppress generation of defaults for LIMIT and OFFSET (#331)

jpa/odata-jpa-processor/src/main/java/com/sap/olingo/jpa/processor/core/api/JPADefaultPagingProvider.java

@@ -4,12 +4,14 @@
 
 import jakarta.persistence.EntityManager;
 
+import org.apache.olingo.commons.api.http.HttpStatusCode;
 import org.apache.olingo.server.api.OData;
 import org.apache.olingo.server.api.ODataApplicationException;
 import org.apache.olingo.server.api.ServiceMetadata;
 import org.apache.olingo.server.api.uri.UriInfo;
 
 import com.sap.olingo.jpa.metadata.api.JPARequestParameterMap;
+import com.sap.olingo.jpa.processor.core.exception.ODataJPAQueryException;
 import com.sap.olingo.jpa.processor.core.query.JPACountQuery;
 
 class JPADefaultPagingProvider implements JPAODataPagingProvider {
@@ -25,9 +27,36 @@ public Optional<JPAODataPage> getFirstPage(final JPARequestParameterMap requestP
       final JPAODataPathInformation pathInformation, final UriInfo uriInfo, final Integer preferredPageSize,
       final JPACountQuery countQuery, final EntityManager em) throws ODataApplicationException {
 
-    final var skipValue = uriInfo.getSkipOption() != null ? uriInfo.getSkipOption().getValue() : 0;
-    final var topValue = uriInfo.getTopOption() != null ? uriInfo.getTopOption().getValue() : Integer.MAX_VALUE;
+//    final SkipOption skipOption = uriResource.getSkipOption();
+//    if (skipOption != null || page != null) {
+//      int skipNumber = skipOption != null ? skipOption.getValue() : page.skip();
+//      skipNumber = skipOption != null && page != null ? Math.max(skipOption.getValue(), page.skip()) : skipNumber;
+//      if (skipNumber >= 0)
+//        typedQuery.setFirstResult(skipNumber);
+//      else
+//        throw new ODataJPAQueryException(ODataJPAQueryException.MessageKeys.QUERY_PREPARATION_INVALID_VALUE,
+//            HttpStatusCode.BAD_REQUEST, Integer.toString(skipNumber), "$skip");
+//    }
+
+    final var skipValue = uriInfo.getSkipOption() != null ? determineSkipValue(uriInfo) : 0;
+    final var topValue = uriInfo.getTopOption() != null ? determineTopValue(uriInfo) : Integer.MAX_VALUE;
     return Optional.of(new JPAODataPage(uriInfo, skipValue, topValue, null));
   }
 
+  private int determineTopValue(final UriInfo uriInfo) throws ODataJPAQueryException {
+    final var value = uriInfo.getTopOption().getValue();
+    if (value < 0)
+      throw new ODataJPAQueryException(ODataJPAQueryException.MessageKeys.QUERY_PREPARATION_INVALID_VALUE,
+          HttpStatusCode.BAD_REQUEST, Integer.toString(value), "$skip");
+    return value;
+  }
+
+  private int determineSkipValue(final UriInfo uriInfo) throws ODataJPAQueryException {
+    final var value = uriInfo.getSkipOption().getValue();
+    if (value < 0)
+      throw new ODataJPAQueryException(ODataJPAQueryException.MessageKeys.QUERY_PREPARATION_INVALID_VALUE,
+          HttpStatusCode.BAD_REQUEST, Integer.toString(value), "$skip");
+    return value;
+  }
+
 }

jpa/odata-jpa-processor/src/main/java/com/sap/olingo/jpa/processor/core/exception/ODataJPAQueryException.java

@@ -37,6 +37,7 @@ public enum MessageKeys implements ODataJPAMessageKey {
     QUERY_PREPARATION_ORDER_BY_NOT_SUPPORTED,
     QUERY_PREPARATION_JOIN_TABLE_TYPE_MISSING,
     QUERY_PREPARATION_COLLECTION_PROPERTY_NOT_SUPPORTED,
+    QUERY_PREPARATION_NO_PAGE_FOUND,
     NOT_SUPPORTED_RESOURCE_TYPE,
     MISSING_CLAIMS_PROVIDER,
     MISSING_CLAIM,

jpa/odata-jpa-processor/src/main/java/com/sap/olingo/jpa/processor/core/processor/JPAProcessorFactory.java

@@ -9,6 +9,8 @@
 import java.util.Map.Entry;
 import java.util.Optional;
 
+import javax.annotation.Nonnull;
+
 import org.apache.olingo.commons.api.ex.ODataException;
 import org.apache.olingo.commons.api.format.ContentType;
 import org.apache.olingo.commons.api.http.HttpHeader;
@@ -100,7 +102,8 @@ public JPARequestProcessor createProcessor(final UriInfo uriInfo, final ContentT
           checkNavigationPathSupported(resourceParts);
           yield new JPANavigationRequestProcessor(odata, serviceMetadata, requestContext);
         }
-        default -> throw new ODataJPAProcessorException(ODataJPAProcessorException.MessageKeys.NOT_SUPPORTED_RESOURCE_TYPE,
+        default -> throw new ODataJPAProcessorException(
+            ODataJPAProcessorException.MessageKeys.NOT_SUPPORTED_RESOURCE_TYPE,
             HttpStatusCode.NOT_IMPLEMENTED, lastItem.getKind().toString());
       };
     } catch (final ODataJPAIllegalAccessException e) {
@@ -128,6 +131,7 @@ private void checkNavigationPathSupported(final List<UriResource> resourceParts)
     }
   }
 
+  @Nonnull
   private JPAODataPage getPage(final Map<String, List<String>> headers, final UriInfo uriInfo,
       final JPAODataRequestContextAccess requestContext, final JPAODataPathInformation pathInformation)
       throws ODataException {

jpa/odata-jpa-processor/src/main/java/com/sap/olingo/jpa/processor/core/query/JPAAbstractJoinQuery.java

@@ -44,8 +44,6 @@
 import org.apache.olingo.server.api.uri.UriResourceProperty;
 import org.apache.olingo.server.api.uri.queryoption.SelectItem;
 import org.apache.olingo.server.api.uri.queryoption.SelectOption;
-import org.apache.olingo.server.api.uri.queryoption.SkipOption;
-import org.apache.olingo.server.api.uri.queryoption.TopOption;
 import org.apache.olingo.server.api.uri.queryoption.expression.ExpressionVisitException;
 
 import com.sap.olingo.jpa.metadata.core.edm.annotation.EdmQueryExtensionProvider;
@@ -450,29 +448,26 @@ private void addCollection(final Map<String, From<?, ?>> joinTables, final JPAPa
   }
 
   private void addSkip(final TypedQuery<Tuple> typedQuery) throws ODataJPAQueryException {
-    final SkipOption skipOption = uriResource.getSkipOption();
-    if (skipOption != null || page != null) {
-      int skipNumber = skipOption != null ? skipOption.getValue() : page.skip();
-      skipNumber = skipOption != null && page != null ? Math.max(skipOption.getValue(), page.skip()) : skipNumber;
-      if (skipNumber >= 0)
-        typedQuery.setFirstResult(skipNumber);
-      else
-        throw new ODataJPAQueryException(ODataJPAQueryException.MessageKeys.QUERY_PREPARATION_INVALID_VALUE,
-            HttpStatusCode.BAD_REQUEST, Integer.toString(skipNumber), "$skip");
+    // Paging provider has to respect $skip. With the JPADefaultPagingProvider there shall be
+    // always a page
+    if (page != null) {
+      if (page.skip() > 0)
+        typedQuery.setFirstResult(page.skip());
+    } else {
+      throw new ODataJPAQueryException(ODataJPAQueryException.MessageKeys.QUERY_PREPARATION_NO_PAGE_FOUND,
+          HttpStatusCode.INTERNAL_SERVER_ERROR, "$skip");
     }
   }
 
-  private void addTop(final TypedQuery<Tuple> tupleQuery) throws ODataJPAQueryException {
-    final TopOption topOption = uriResource.getTopOption();
-    if (topOption != null || page != null) {
-      int topNumber = topOption != null ? topOption.getValue() : page.top();
-      topNumber = topOption != null && page != null ? Math.min(topOption.getValue(), page.top())
-          : topNumber;
-      if (topNumber >= 0)
-        tupleQuery.setMaxResults(topNumber);
-      else
-        throw new ODataJPAQueryException(ODataJPAQueryException.MessageKeys.QUERY_PREPARATION_INVALID_VALUE,
-            HttpStatusCode.BAD_REQUEST, Integer.toString(topNumber), "$top");
+  private void addTop(final TypedQuery<Tuple> typedQuery) throws ODataJPAQueryException {
+    // Paging provider has to respect $top. With the JPADefaultPagingProvider there shall be
+    // always a page
+    if (page != null) {
+      if (page.top() < Integer.MAX_VALUE)
+        typedQuery.setMaxResults(page.top());
+    } else {
+      throw new ODataJPAQueryException(ODataJPAQueryException.MessageKeys.QUERY_PREPARATION_NO_PAGE_FOUND,
+          HttpStatusCode.INTERNAL_SERVER_ERROR, "$top");
     }
   }
 

jpa/odata-jpa-processor/src/main/java/com/sap/olingo/jpa/processor/core/query/JPAExpandFilterQuery.java

@@ -270,15 +270,15 @@ Expression<Boolean> createWhereSubQuery(@Nullable final Subquery<?> childQuery,
 
   private Integer getSkipValue(@Nullable final Subquery<?> childQuery) {
     if (navigationInfo.getPage() != null)
-      return navigationInfo.getPage().skip();
+      return navigationInfo.getPage().skip() > 0 ? navigationInfo.getPage().skip() : null;
     if (navigationInfo.getUriInfo().getSkipOption() != null && childQuery == null)
       return navigationInfo.getUriInfo().getSkipOption().getValue();
     return null;
   }
 
   private Integer getTopValue(@Nullable final Subquery<?> childQuery) {
     if (navigationInfo.getPage() != null)
-      return navigationInfo.getPage().top();
+      return navigationInfo.getPage().top() < Integer.MAX_VALUE ? navigationInfo.getPage().top() : null;
     if (navigationInfo.getUriInfo().getTopOption() != null && childQuery == null)
       return navigationInfo.getUriInfo().getTopOption().getValue();
     return null;

jpa/odata-jpa-processor/src/main/resources/processor-exceptions-i18n.properties

@@ -79,6 +79,7 @@ ODataJPAQueryException.QUERY_PREPARATION_NOT_ALLOWED_MEMBER = Not authorized to
 ODataJPAQueryException.QUERY_PREPARATION_ORDER_BY_TRANSIENT= Usage of '%1$s' within OrderBy clauses not supported
 ODataJPAQueryException.QUERY_PREPARATION_JOIN_TABLE_TYPE_MISSING=The expand implementation requires that a join table ('%1$s') has an entity.
 ODataJPAQueryException.QUERY_PREPARATION_COLLECTION_PROPERTY_NOT_SUPPORTED=Filter with collection property not supported
+ODataJPAQueryException.QUERY_PREPARATION_NO_PAGE_FOUND=No page found while converting '%1$s'
 ODataJPAQueryException.NOT_SUPPORTED_RESOURCE_TYPE = Resource type '%1$s' not supported
 ODataJPAQueryException.MISSING_CLAIMS_PROVIDER = Authorization information missing
 ODataJPAQueryException.MISSING_CLAIM = Authorization information missing for at least one property

jpa/odata-jpa-processor/src/test/java/com/sap/olingo/jpa/processor/core/api/JPADefaultPagingProviderTest.java

@@ -2,6 +2,7 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 import static org.mockito.Mockito.mock;
@@ -14,6 +15,8 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import com.sap.olingo.jpa.processor.core.exception.ODataJPAQueryException;
+
 class JPADefaultPagingProviderTest {
 
   private JPAODataPagingProvider cut;
@@ -76,4 +79,17 @@ void testGetFirstPageReturnsTopAndSkipAsRequested() throws ODataApplicationExcep
     assertNull(act.get().skipToken());
   }
 
+  @Test
+  void testGetFirstPageThrowsExceptionSkipNegative() throws ODataApplicationException {
+    when(skipOption.getValue()).thenReturn(-99);
+    when(uriInfo.getSkipOption()).thenReturn(skipOption);
+    assertThrows(ODataJPAQueryException.class, () -> cut.getFirstPage(null, null, uriInfo, null, null, null));
+  }
+
+  @Test
+  void testGetFirstPageThrowsExceptionTopNegative() throws ODataApplicationException {
+    when(topOption.getValue()).thenReturn(-99);
+    when(uriInfo.getTopOption()).thenReturn(topOption);
+    assertThrows(ODataJPAQueryException.class, () -> cut.getFirstPage(null, null, uriInfo, null, null, null));
+  }
 }

jpa/odata-jpa-processor/src/test/java/com/sap/olingo/jpa/processor/core/api/JPAODataContextAccessDouble.java

@@ -34,7 +34,7 @@ public JPAODataContextAccessDouble(final JPAEdmProvider edmProvider, final DataS
     this.dataSource = dataSource;
     this.processor = new JPADefaultDatabaseProcessor();
     this.packageNames = packages;
-    this.pagingProvider = provider;
+    this.pagingProvider = provider != null ? provider : new JPADefaultPagingProvider();
     this.annotationProvider = annotationProvider;
     try {
       this.directives = JPAODataServiceContext.with().useQueryDirectives().maxValuesInInClause(3).build().build()

jpa/odata-jpa-processor/src/test/java/com/sap/olingo/jpa/processor/core/api/JPAODataServiceContextBuilderTest.java

@@ -108,11 +108,10 @@ void checkReturnsProvidedPagingProvider() throws ODataException {
   }
 
   @Test
-  void checkReturnsDefaultProvidedPagingIfNotProvider() throws ODataException {
+  void checkReturnsDefaultPagingProviderIfNotProvider() throws ODataException {
     cut = JPAODataServiceContext.with()
         .setDataSource(dataSource)
         .setPUnit(PUNIT_NAME)
-        .setPagingProvider(null)
         .build();
 
     assertTrue(cut.getPagingProvider() instanceof JPADefaultPagingProvider);

jpa/odata-jpa-processor/src/test/java/com/sap/olingo/jpa/processor/core/query/JPAJoinQueryTest.java

@@ -1,23 +1,26 @@
 package com.sap.olingo.jpa.processor.core.query;
 
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.when;
 
 import jakarta.persistence.EntityManager;
 import jakarta.persistence.criteria.CriteriaBuilder;
-import jakarta.persistence.criteria.CriteriaQuery;
 
 import org.apache.olingo.commons.api.ex.ODataException;
+import org.apache.olingo.server.api.ODataApplicationException;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
 import com.sap.olingo.jpa.metadata.api.JPAEdmProvider;
 import com.sap.olingo.jpa.metadata.api.JPAHttpHeaderMap;
 import com.sap.olingo.jpa.metadata.api.JPARequestParameterMap;
+import com.sap.olingo.jpa.metadata.core.edm.mapper.api.JPAEntityType;
 import com.sap.olingo.jpa.metadata.core.edm.mapper.api.JPAStructuredType;
+import com.sap.olingo.jpa.metadata.core.edm.mapper.exception.ODataJPAModelException;
 import com.sap.olingo.jpa.metadata.core.edm.mapper.impl.JPADefaultEdmNameBuilder;
 import com.sap.olingo.jpa.processor.core.api.JPAODataRequestContextAccess;
 import com.sap.olingo.jpa.processor.core.database.JPADefaultDatabaseProcessor;
@@ -30,8 +33,7 @@
 
 class JPAJoinQueryTest extends TestQueryBase {
   private CriteriaBuilder cb;
-  @SuppressWarnings("rawtypes")
-  private CriteriaQuery cq;
+
   private EntityManager em;
   private JPAODataRequestContextAccess localContext;
   private JPAHttpHeaderMap headerMap;
@@ -42,7 +44,7 @@ class JPAJoinQueryTest extends TestQueryBase {
   public void setup() throws ODataException, ODataJPAIllegalAccessException {
     em = mock(EntityManager.class);
     cb = spy(emf.getCriteriaBuilder());
-    cq = mock(CriteriaQuery.class);
+
     localContext = mock(JPAODataRequestContextAccess.class);
     headerMap = mock(JPAHttpHeaderMap.class);
     parameterMap = mock(JPARequestParameterMap.class);
@@ -111,4 +113,11 @@ void testDerivedTypeRequestedFalseOtherBaseType() {
 
     assertFalse(cut.derivedTypeRequested(baseType, potentialSubType));
   }
+
+  @Test
+  void testBuildEntityPathListRethrowsException() throws ODataApplicationException, ODataJPAModelException {
+    final var jpaEntityType = mock(JPAEntityType.class);
+    when(jpaEntityType.getPathList()).thenThrow(ODataJPAModelException.class);
+    assertThrows(ODataApplicationException.class, () -> cut.buildEntityPathList(jpaEntityType));
+  }
 }

jpa/odata-jpa-processor/src/test/java/com/sap/olingo/jpa/processor/core/query/TestJPAQueryTopSkip.java

@@ -17,6 +17,7 @@
 import org.apache.olingo.commons.api.ex.ODataException;
 import org.apache.olingo.server.api.OData;
 import org.apache.olingo.server.api.debug.DefaultDebugSupport;
+import org.junit.jupiter.api.Tag;
 import org.junit.jupiter.api.Test;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
@@ -27,6 +28,7 @@
 import com.sap.olingo.jpa.processor.core.api.JPAODataRequestProcessor;
 import com.sap.olingo.jpa.processor.core.api.JPAODataServiceContext;
 import com.sap.olingo.jpa.processor.core.processor.JPAODataInternalRequestContext;
+import com.sap.olingo.jpa.processor.core.util.Assertions;
 import com.sap.olingo.jpa.processor.core.util.IntegrationTestHelper;
 import com.sap.olingo.jpa.processor.core.util.TestBase;
 
@@ -107,6 +109,17 @@ void testTopReturnsAllIfToLarge() throws IOException, ODataException {
 
   }
 
+  @Tag(Assertions.CB_ONLY_TEST)
+  @Test
+  void testExpandTopSkipWithoutError() throws IOException, ODataException {
+    final IntegrationTestHelper helper = new IntegrationTestHelper(emf,
+        "AdministrativeDivisions?$skip=5&$top=5&$expand=Children");
+    helper.assertStatus(200);
+    final ObjectNode collection = helper.getValue();
+    final ArrayNode act = ((ArrayNode) collection.get("value"));
+    assertEquals(5, act.size());
+  }
+
   public String getRawResult(final HttpServletResponse response) throws IOException {
     final InputStream in = asInputStream(response);
     final StringBuilder builder = new StringBuilder();

jpa/odata-jpa-processor/src/test/java/com/sap/olingo/jpa/processor/core/query/TestJPAServerDrivenPaging.java

@@ -52,10 +52,10 @@
 
 class TestJPAServerDrivenPaging extends TestBase {
   @Test
-  void testReturnsNotImplementedIfPagingProviderNotAvailable() throws IOException, ODataException {
+  void testReturnsGoneIfPagingProviderNotAvailable() throws IOException, ODataException {
 
     final IntegrationTestHelper helper = new IntegrationTestHelper(emf, "Organizations?$skiptoken=xyz");
-    helper.assertStatus(501);
+    helper.assertStatus(410);
   }
 
   @Test