Fix paging and add sorting

Limit/offset wasn't being applied correctly. Added sorting through the sortby paramter.
georchestra · Nov 1, 2023 · 14b258b · 14b258b
1 parent 77fe100
commit 14b258b
Show file tree

Hide file tree

Showing 9 changed files with 207 additions and 43 deletions.
diff --git a/...-functions/data-functions-core/src/main/java/com/camptocamp/opendata/model/DataQuery.java b/...-functions/data-functions-core/src/main/java/com/camptocamp/opendata/model/DataQuery.java
@@ -1,6 +1,7 @@
 package com.camptocamp.opendata.model;
 
 import java.net.URI;
+import java.util.Comparator;
 import java.util.List;
 
 import lombok.Builder;
@@ -27,7 +28,26 @@ public static DataQuery fromUri(URI dataUri) {
         return DataQuery.builder().source(dataSource).sortBy(List.of()).build();
     }
 
-    public static record SortBy(String propertyName, boolean ascending) {
+    public static record SortBy(String propertyName, boolean ascending) implements Comparator<GeodataRecord> {
 
+    @SuppressWarnings("unchecked")
+    @Override
+    public int compare(GeodataRecord o1, GeodataRecord o2) {
+        Comparable<Object> p1 = (Comparable<Object>) o1.getProperty(propertyName).map(SimpleProperty::getValue)
+                .orElse(null);
+        Comparable<Object> p2 = (Comparable<Object>) o2.getProperty(propertyName).map(SimpleProperty::getValue)
+                .orElse(null);
+
+        if (p1 == null && p2 == null)
+            return 0;
+        if (p1 == null)
+            return ascending ? 1 : -1;
+        if (p2 == null)
+            return ascending ? -1 : 1;
+
+        if (ascending)
+            return p1.compareTo(p2);
+        return p2.compareTo(p1);
     }
-}
+
+}}
diff --git a/...tocamp/opendata/ogc/features/http/codec/csv/CsvFeatureCollectionHttpMessageConverter.java b/...tocamp/opendata/ogc/features/http/codec/csv/CsvFeatureCollectionHttpMessageConverter.java
@@ -86,9 +86,9 @@ public CsvFeatureCollectionHttpMessageConverter() {
         CSVWriter writer = new CSVWriter(new OutputStreamWriter(body), separator, quotechar, escapechar, lineSeparator);
 
         AtomicBoolean headerWritten = new AtomicBoolean();
-        message.getFeatures().forEach(rec -> {
-            encodeValue(rec, writer, headerWritten);
-        });
+        try (Stream<GeodataRecord> features = message.getFeatures()) {
+            features.forEach(rec -> encodeValue(rec, writer, headerWritten));
+        }
         writer.flush();
         body.flush();
     }

diff --git a/.../opendata/ogc/features/http/codec/xls/Excel2007FeatureCollectionHttpMessageConverter.java b/.../opendata/ogc/features/http/codec/xls/Excel2007FeatureCollectionHttpMessageConverter.java
@@ -16,6 +16,7 @@
 import org.springframework.http.converter.HttpMessageNotWritableException;
 import org.springframework.util.MimeType;
 
+import com.camptocamp.opendata.model.GeodataRecord;
 import com.camptocamp.opendata.model.SimpleProperty;
 import com.camptocamp.opendata.ogc.features.http.codec.MimeTypes;
 import com.camptocamp.opendata.ogc.features.http.codec.xls.StreamingWorkbookWriter.StreamingRow;
@@ -59,15 +60,17 @@ public Excel2007FeatureCollectionHttpMessageConverter() {
         StreamingWorkbookWriter writer = new StreamingWorkbookWriter(outputMessage.getBody());
         try {
             addHeader(writer, message.getOriginalContents());
-            message.getFeatures().forEach(rec -> {
-                StreamingRow row = writer.newRow();
-                row.addColumnValue(rec.getId());
-                rec.getProperties().stream().map(SimpleProperty::getValue).forEach(row::addColumnValue);
-                if (null != rec.getGeometry()) {
-                    row.addColumnValue(rec.getGeometry().getValue());
-                }
-                row.end();
-            });
+            try (Stream<GeodataRecord> features = message.getFeatures()) {
+                features.forEach(rec -> {
+                    StreamingRow row = writer.newRow();
+                    row.addColumnValue(rec.getId());
+                    rec.getProperties().stream().map(SimpleProperty::getValue).forEach(row::addColumnValue);
+                    if (null != rec.getGeometry()) {
+                        row.addColumnValue(rec.getGeometry().getValue());
+                    }
+                    row.end();
+                });
+            }
         } finally {
             writer.finish();
         }

diff --git a/...n/java/com/camptocamp/opendata/ogc/features/repository/DataStoreCollectionRepository.java b/...n/java/com/camptocamp/opendata/ogc/features/repository/DataStoreCollectionRepository.java
@@ -8,6 +8,7 @@
 import java.util.Optional;
 import java.util.concurrent.Callable;
 import java.util.function.Function;
+import java.util.stream.Collectors;
 
 import org.geotools.api.data.DataStore;
 import org.geotools.api.data.Query;
@@ -31,6 +32,7 @@
 import com.camptocamp.opendata.ogc.features.model.Collection;
 import com.camptocamp.opendata.ogc.features.model.FeatureCollection;
 import com.camptocamp.opendata.ogc.features.model.GeoToolsFeatureCollection;
+import com.google.common.base.Throwables;
 
 import lombok.NonNull;
 import lombok.RequiredArgsConstructor;
@@ -88,7 +90,12 @@ private <T> T runWithRetry(String description, Callable<T> command) {
         try {
             return command.call();
         } catch (Exception e) {
-            log.info("Retrying command %s".formatted(description));
+            String message = Throwables.getRootCause(e).getMessage();
+            if (log.isDebugEnabled()) {
+                log.info("Retrying command %s: %s".formatted(description, message), e);
+            } else {
+                log.info("Retrying command %s: %s".formatted(description, message));
+            }
             dataStoreProvider.reInit();
             try {
                 return command.call();
@@ -109,7 +116,7 @@ public FeatureCollection query(@NonNull DataQuery query) {
         return runWithRetry("query(%s)".formatted(query.getLayerName()), () -> {
             ensureSchemaIsInSync(gtQuery);
             SimpleFeatureCollection fc = query(gtQuery);
-            long matched = count(toQuery(query.withLimit(null)));
+            long matched = count(toQuery(query.withLimit(null).withOffset(null)));
             long returned = count(gtQuery);
             GeoToolsFeatureCollection ret = new GeoToolsFeatureCollection(collection, fc);
             ret.setNumberMatched(matched);
@@ -143,9 +150,12 @@ private int count(Query query) {
 
     private Query toQuery(@NonNull DataQuery query) {
         Query q = new Query(query.getLayerName());
-        if (null != query.getLimit())
+        Integer limit = query.getLimit();
+        Integer offset = query.getOffset();
+
+        if (null != limit)
             q.setMaxFeatures(query.getLimit());
-        if (null != query.getOffset())
+        if (null != offset)
             q.setStartIndex(query.getOffset());
         if (null != query.getFilter()) {
             try {
@@ -155,21 +165,20 @@ private Query toQuery(@NonNull DataQuery query) {
             }
         }
         List<SortBy> sortBy = sortBy(query);
-        if (sortBy.isEmpty()) {
-            // default to natural order for paging consistency
-            q.setSortBy(SortBy.NATURAL_ORDER);
-        } else {
-            q.setSortBy(sortBy.toArray(SortBy[]::new));
+        if (null != limit || null != offset) {
+            // always add natural order for paging consistency
+            sortBy.add(SortBy.NATURAL_ORDER);
         }
+        q.setSortBy(sortBy.toArray(SortBy[]::new));
         return q;
     }
 
-    public List<SortBy> sortBy(DataQuery q) {
-        return q.getSortBy().stream().map(this::toSortBy).toList();
+    private List<SortBy> sortBy(DataQuery q) {
+        return q.getSortBy().stream().map(this::toSortBy).collect(Collectors.toList());
     }
 
     private SortBy toSortBy(DataQuery.SortBy order) {
-        return ff.sort(order.propertyName(), order.ascending() ? SortOrder.DESCENDING : SortOrder.DESCENDING);
+        return ff.sort(order.propertyName(), order.ascending() ? SortOrder.ASCENDING : SortOrder.DESCENDING);
     }
 
     private SimpleFeatureCollection query(Query query) {

diff --git a/...es/src/main/java/com/camptocamp/opendata/ogc/features/server/impl/CollectionsApiImpl.java b/...es/src/main/java/com/camptocamp/opendata/ogc/features/server/impl/CollectionsApiImpl.java
@@ -86,8 +86,21 @@ public ResponseEntity<FeatureCollection> getFeatures(//
             String filterCrs, //
             List<String> sortby) {
 
-        FeaturesQuery fq = FeaturesQuery.of(collectionId).withLimit(limit).withBbox(bbox).withDatetime(datetime)
-                .withFilter(filter).withFilterLang(filterLang).withFilterCrs(filterCrs);
+        NativeWebRequest request = getRequest().orElseThrow();
+        Integer offset = extractOffset(request);
+
+        if (limit != null && limit.intValue() < 0) {
+            limit = null;
+        }
+        FeaturesQuery fq = FeaturesQuery.of(collectionId)//
+                .withLimit(limit)//
+                .withOffset(offset)//
+                .withBbox(bbox)//
+                .withDatetime(datetime)//
+                .withFilter(filter)//
+                .withFilterLang(filterLang)//
+                .withFilterCrs(filterCrs)//
+                .withSortby(sortby);
         return getFeatures(fq);
     }
 
@@ -211,8 +224,7 @@ private Link link(String href, String rel, String type, String title) {
 
     DataQuery toDataQuery(FeaturesQuery query) {
 
-        List<DataQuery.SortBy> sortby = Optional.ofNullable(query.getSortby()).orElse(List.of()).stream()
-                .map(this::toSortBy).toList();
+        List<DataQuery.SortBy> sortby = query.sortBy();
 
         DataQuery q = DataQuery.fromUri(URI.create("index://default"))//
                 .withLayerName(query.getCollectionId())//
@@ -223,16 +235,4 @@ DataQuery toDataQuery(FeaturesQuery query) {
         return q;
     }
 
-    private DataQuery.SortBy toSortBy(String s) {
-        String propertyName = s;
-        boolean ascending = true;
-        if (s.startsWith("+")) {
-            propertyName = propertyName.substring(1);
-        } else if (s.startsWith("-")) {
-            ascending = false;
-            propertyName = propertyName.substring(1);
-        }
-        return new DataQuery.SortBy(propertyName, ascending);
-    }
-
 }
diff --git a/...eatures/src/main/java/com/camptocamp/opendata/ogc/features/server/impl/FeaturesQuery.java b/...eatures/src/main/java/com/camptocamp/opendata/ogc/features/server/impl/FeaturesQuery.java
@@ -2,10 +2,14 @@
 
 import java.math.BigDecimal;
 import java.util.List;
+import java.util.Optional;
 
 import org.geotools.api.filter.FilterFactory;
 import org.geotools.factory.CommonFactoryFinder;
 
+import com.camptocamp.opendata.model.DataQuery;
+import com.camptocamp.opendata.model.DataQuery.SortBy;
+
 import lombok.NonNull;
 import lombok.RequiredArgsConstructor;
 import lombok.Value;
@@ -36,4 +40,21 @@ public FeaturesQuery withBbox(double minx, double miny, double maxx, double maxy
         return withBbox(List.of(BigDecimal.valueOf(minx), BigDecimal.valueOf(miny), BigDecimal.valueOf(maxx),
                 BigDecimal.valueOf(maxy)));
     }
+
+    public List<SortBy> sortBy() {
+        return Optional.ofNullable(getSortby()).orElse(List.of()).stream().map(this::toSortBy).toList();
+    }
+
+    private DataQuery.SortBy toSortBy(String s) {
+        String propertyName = s;
+        boolean ascending = true;
+        if (s.startsWith("+")) {
+            propertyName = propertyName.substring(1);
+        } else if (s.startsWith("-")) {
+            ascending = false;
+            propertyName = propertyName.substring(1);
+        }
+        return new DataQuery.SortBy(propertyName, ascending);
+    }
+
 }
diff --git a/...t/java/com/camptocamp/opendata/ogc/features/server/impl/AbstractCollectionsApiImplIT.java b/...t/java/com/camptocamp/opendata/ogc/features/server/impl/AbstractCollectionsApiImplIT.java
@@ -1,21 +1,31 @@
 package com.camptocamp.opendata.ogc.features.server.impl;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.Assert.fail;
 
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.LinkedHashSet;
+import java.util.List;
 import java.util.Set;
 import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.data.domain.Pageable;
 import org.springframework.http.ResponseEntity;
 import org.springframework.mock.web.MockHttpServletRequest;
 import org.springframework.web.context.request.NativeWebRequest;
 
+import com.camptocamp.opendata.model.DataQuery.SortBy;
+import com.camptocamp.opendata.model.GeodataRecord;
 import com.camptocamp.opendata.ogc.features.model.Collection;
 import com.camptocamp.opendata.ogc.features.model.Collections;
 import com.camptocamp.opendata.ogc.features.model.FeatureCollection;
+import com.google.common.base.Splitter;
 
 public abstract class AbstractCollectionsApiImplIT {
 
@@ -60,4 +70,86 @@ public void testGetItemsWithFilter() {
 
         assertThat(response.getBody().getFeatures().count()).isEqualTo(1);
     }
+
+    @ParameterizedTest
+    @ValueSource(strings = { "base-sirene-v3", "comptages-velo", "locations", "ouvrages-acquis-par-les-mediatheques" })
+    public void testGetItems_paging_natural_order(String layerName) {
+        FeaturesQuery query = FeaturesQuery.of(layerName);
+
+        Comparator<GeodataRecord> fidComparator = fidComparator();
+        testPagingConsistency(query, fidComparator);
+    }
+
+    protected abstract Comparator<GeodataRecord> fidComparator();
+
+    @ParameterizedTest
+    @ValueSource(strings = { //
+            "base-sirene-v3:+nic,-siren", //
+            "comptages-velo:commune,date et heure", //
+            "locations:-city", //
+            "ouvrages-acquis-par-les-mediatheques:-type de document,editeur" })
+    public void testGetItems_paging_mixed_order(String layerAndSortSpec) {
+        String layerName = layerAndSortSpec.substring(0, layerAndSortSpec.indexOf(':'));
+        String sortSpec = layerAndSortSpec.substring(1 + layerAndSortSpec.indexOf(':'));
+
+        List<String> sortby = Splitter.on(',').trimResults().omitEmptyStrings().splitToList(sortSpec);
+
+        FeaturesQuery query = FeaturesQuery.of(layerName).withSortby(sortby);
+
+        Comparator<GeodataRecord> comparator = sortSpecComparator(query.sortBy());
+        testPagingConsistency(query, comparator);
+    }
+
+    protected Comparator<GeodataRecord> sortSpecComparator(List<SortBy> sortBy) {
+        Comparator<GeodataRecord> comparator = sortBy.get(0);
+        for (int i = 1; i < sortBy.size(); i++) {
+            comparator = comparator.thenComparing(sortBy.get(i));
+        }
+        return comparator.thenComparing(fidComparator());
+    }
+
+    private void testPagingConsistency(FeaturesQuery query, Comparator<GeodataRecord> comparator) {
+
+        final int total = collectionsApi.getFeatures(FeaturesQuery.of(query.getCollectionId())).getBody()
+                .getNumberMatched().intValue();
+        final int pageSize = total / 11;
+        final int lastPageSize = Math.min(pageSize, total % pageSize);
+        final int pages = total / pageSize + (lastPageSize > 0 ? 1 : 0);
+        query = query.withLimit(pageSize);
+
+        final Pageable last = Pageable.ofSize(lastPageSize == 0 ? pageSize : lastPageSize).withPage(pages - 1);
+
+        Pageable page = Pageable.ofSize(pageSize);
+        Set<String> idsReturned = new LinkedHashSet<>();
+        List<GeodataRecord> records = new ArrayList<>();
+
+        for (int p = 0; p < pages; p++, page = page.next()) {
+            int offset = (int) page.getOffset();
+            query = query.withOffset(offset);
+            FeatureCollection collection = collectionsApi.getFeatures(query).getBody();
+            assertThat(collection.getNumberMatched())
+                    .as("numberMatched should be the number of features matching the query filter").isEqualTo(total);
+
+            int pageNumber = page.getPageNumber();
+            if (pageNumber == last.getPageNumber()) {
+                assertThat(collection.getNumberReturned()).as("numberReturned should be equal to last page size")
+                        .isEqualTo(last.getPageSize());
+            } else {
+                assertThat(collection.getNumberReturned()).as("numberReturned should be equal to page size")
+                        .isEqualTo(page.getPageSize());
+            }
+            try (Stream<GeodataRecord> features = collection.getFeatures()) {
+                features.forEach(rec -> {
+                    String id = rec.getId();
+                    assertThat(idsReturned).as("Duplicate feature returned on page " + pageNumber).doesNotContain(id);
+                    idsReturned.add(id);
+                    records.add(rec);
+                });
+            }
+        }
+        // verify expected order
+//		var sorted = new ArrayList<>(records);
+//		java.util.Collections.sort(sorted, comparator);
+//		assertThat(records).isEqualTo(sorted);
+    }
 }