#103 - adding pagination #104

pveeckhout · 2023-08-29T23:43:03Z

closes #103

…yfilter-endpoint

officialyinsane · 2023-09-04T20:54:35Z

exploration-module/src/main/java/io/edpn/backend/exploration/adapter/config/AsyncConfig.java

-        return taskExecutor;
+    @Bean(name = "explorationForkJoinPool")
+    public ForkJoinPool forkJoinPool() {
+        return new ForkJoinPool(50); // todo set from config


Magic number and a TODO tag, should we not have these summarised in some issue? Maybe deal with it all under one issue?

Ideally, should be configurable.

officialyinsane · 2023-09-04T20:58:11Z

...main/java/io/edpn/backend/trade/adapter/persistence/entity/mapper/MybatisPageInfoMapper.java

+import io.edpn.backend.trade.application.dto.persistence.entity.PersistencePageInfo;
+import io.edpn.backend.trade.application.dto.persistence.entity.mapper.PersistencePageInfoMapper;
+
+public class MybatisPageInfoMapper implements PersistencePageInfoMapper {


Nothing in this class is specific to Trade, but could be useful for other modules... perhaps we should consider adding a "common" module for things like this?

this would mean breaking changes for all modules, one change in will ripple to all. the modules are not versioned independently in the modulith

this would mean breaking changes for all modules, one change in will ripple to all. the modules are not versioned independently in the modulith

Well, is there something keeping us from versioning them independently?

officialyinsane · 2023-09-04T20:59:50Z

...a/io/edpn/backend/trade/adapter/persistence/repository/MybatisLocateCommodityRepository.java

+            						require_odyssey,
+            						fleet_carrier
+            				 FROM filtered_lcv
+            				 WHERE 1 = 1


We still have WHERE 1=1 - not critical though.

here it is needed, all following AND clauses can be filtered out and we need one initial where otherwise there is a syntax error

officialyinsane · 2023-09-04T21:01:55Z

...c/main/java/io/edpn/backend/trade/adapter/web/dto/filter/mapper/RestPageFilterDtoMapper.java

+
+    private PageFilter.PageFilterBuilder getDefaultFilterBuilder() {
+        return PageFilter.builder()
+                .size(20) //TODO get from config


Magic number again that should be configurable as a defaultPageSize etc.

…tion-to-locate-commodityfilter-endpoint

…yfilter-endpoint

…tion-to-locate-commodityfilter-endpoint

Daniel-J-Mason · 2023-09-14T10:53:05Z

trade-module/src/main/java/io/edpn/backend/trade/adapter/config/ServiceConfig.java

@@ -228,7 +230,7 @@ public RequestMissingSystemCoordinatesService requestMissingSystemCoordinatesUse
            CreateSystemCoordinateRequestPort createSystemCoordinateRequestPort,
            SendKafkaMessagePort sendKafkaMessagePort,
            @Qualifier("tradeRetryTemplate") RetryTemplate retryTemplate,
-            @Qualifier("tradeThreadPoolTaskExecutor") Executor executor,
+            @Qualifier("tradeForkJoinPool") Executor executor,


I cant comment the line apparently, line 246 for tradeRequestMissingStationRequireOdysseyUseCase bean has an invalid executor qualifier now. Revise

Daniel-J-Mason · 2023-09-14T10:58:17Z

...src/main/java/io/edpn/backend/trade/adapter/web/dto/filter/RestLocateCommodityFilterDto.java

        Long minSupply,
-        Long minDemand) implements LocateCommodityFilterDto {
+        Long minDemand,
+        RestPageFilterDto page


This currently does not work. Im not sure if we can fix this with @Jacksonized. But with a corrected http request this throws mapping errors. Consider flattening and converting to a domain object via mapper, or investigate a way to make this work with a get request

Daniel-J-Mason · 2023-09-14T10:59:46Z

trade-module/src/main/java/io/edpn/backend/trade/adapter/web/TradeModuleController.java

-
-    @GetMapping("/locate-commodity/filter")
-    public List<RestLocateCommodityDto> locateCommodityWithFilters(RestLocateCommodityFilterDto locateCommodityFilterDto){
+    @ApiResponses({


This endpoint does not work. [] do not encode properly in tomcat 8.5+, http encrypts [ = %5B and ] = %5D
Find a way to fix if we are going to maintain this structure for the endpoint. This is potentially a problem on the swagger side, but requires a bit more digging

…tion-to-locate-commodityfilter-endpoint

jakaarl

I'm a bit at a loss about where enums, records and such can or cannot be used and why. Doesn't seem very consistent to me. 😕

jakaarl · 2023-10-07T08:08:35Z

...module/src/main/java/io/edpn/backend/trade/adapter/persistence/filter/MybatisPageFilter.java

+@AllArgsConstructor
+@NoArgsConstructor
+@Builder
+@EqualsAndHashCode


As much as I hate writing boilerplate, this sort of ADD (not attention deficit disorder, Annotation Driven Development!) makes me cringe, too. IIRC, we have some limitations wrt. using records, but do they apply here?

jakaarl · 2023-10-07T08:19:39Z

...a/io/edpn/backend/trade/adapter/persistence/repository/MybatisLocateCommodityRepository.java

+            		OFFSET (#{page.size} * #{page.page})
+            	</if>
+            </script>
+            """


I honestly don't get why annotations with embedded scripting generating SQL would be a good idea. Personally, would choose a type-safe programmatic approach any given day. JOOQ or QueryDSL or something.

Please, at least tell me the validity and correctness of the scripts are checked at compile time!

Disclaimer: I don't have any prior experience on MyBatis. In fact, been trying to read up on it, but their own documentation seems a bit... thin. Could use a few pointers or a reading list.

jakaarl · 2023-10-07T08:24:06Z

.../io/edpn/backend/trade/adapter/web/dto/filter/mapper/RestLocateCommodityFilterDtoMapper.java

+
+    private static LandingPadSize getShipSize(LocateCommodityFilterDto locateCommodityFilterDto) {
+        return Optional.ofNullable(locateCommodityFilterDto.shipSize())
+                .map(LandingPadSize::valueOf)


Seems like the enum is being converted back and forth - I'm guessing enum is not an option for DTOs because reasons?

jakaarl · 2023-10-07T08:32:20Z

trade-module/src/main/java/io/edpn/backend/trade/application/domain/PageInfo.java

+@ToString
+@AllArgsConstructor
+@Builder
+public class PageInfo {


Guessing this could be a record, since PagedResult right next door is...

jakaarl · 2023-10-07T08:34:35Z

...src/main/java/io/edpn/backend/trade/application/dto/web/filter/LocateCommodityFilterDto.java

    Double yCoordinate();
-
+
    Double zCoordinate();


BTW do X, Y and Z coordinates make much sense independently? If not, shouldn't we have a Coordinates object instead?

jakaarl · 2023-10-07T08:37:08Z

...st/java/io/edpn/backend/trade/adapter/web/dto/filter/mapper/RestPageFilterDtoMapperTest.java

+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+@ExtendWith(MockitoExtension.class)


Looks like MockitoExtension isn't really used here?

#103 - adding pagination

4ce3f74

pveeckhout added the enhancement New feature or request label Aug 29, 2023

pveeckhout requested review from jakaarl, Daniel-J-Mason, officialyinsane and a team August 29, 2023 23:43

pveeckhout self-assigned this Aug 29, 2023

pveeckhout linked an issue Aug 29, 2023 that may be closed by this pull request

add pagination to /locate-commodity/filter endpoint #103

Open

Pieter Van Eeckhout and others added 3 commits August 30, 2023 01:47

#103 - changing to implementation to facilitate swagger documentation

5dfa58d

#103 - adding schema

a512ec5

#103 - wip returning pageInfo in result

c241636

pveeckhout marked this pull request as draft September 1, 2023 08:25

pveeckhout added 2 commits September 4, 2023 09:34

Merge branch 'development' into 103-add-pagination-to-locate-commodit…

596f0b7

…yfilter-endpoint

#103 - improving pageInfo returned

f856272

pveeckhout marked this pull request as ready for review September 4, 2023 15:20

pveeckhout added 2 commits September 4, 2023 17:20

Merge branch 'development' into 103-add-pagination-to-locate-commodit…

bd97a6e

…yfilter-endpoint

Merge branch 'development' into 103-add-pagination-to-locate-commodit…

1a54e03

…yfilter-endpoint

officialyinsane approved these changes Sep 4, 2023

View reviewed changes

Pieter Van Eeckhout and others added 3 commits September 5, 2023 01:59

Merge remote-tracking branch 'origin/development' into 103-add-pagina…

c9e3559

…tion-to-locate-commodityfilter-endpoint

Merge branch 'development' into 103-add-pagination-to-locate-commodit…

20b4ab8

…yfilter-endpoint

Merge remote-tracking branch 'origin/development' into 103-add-pagina…

b1db673

…tion-to-locate-commodityfilter-endpoint

Daniel-J-Mason requested changes Sep 14, 2023

View reviewed changes

pveeckhout added 4 commits September 14, 2023 13:47

#103 - fixing outdated qualifier

800bebc

#103 -adding paging request info directly on the filter object

0fc099a

#103 - fixing generics issue with swagger

8bd8cde

Merge remote-tracking branch 'origin/development' into 103-add-pagina…

a9c494f

…tion-to-locate-commodityfilter-endpoint

pveeckhout marked this pull request as draft September 15, 2023 13:40

aslink87 mentioned this pull request Sep 22, 2023

implement pagination/limits/both into commodity form responses ed-pilots-network/frontend#100

Open

jakaarl reviewed Oct 7, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#103 - adding pagination #104

#103 - adding pagination #104

pveeckhout commented Aug 29, 2023

officialyinsane Sep 4, 2023

officialyinsane Sep 4, 2023

pveeckhout Sep 4, 2023

jakaarl Oct 7, 2023

officialyinsane Sep 4, 2023

pveeckhout Sep 4, 2023

officialyinsane Sep 4, 2023

Daniel-J-Mason Sep 14, 2023

Daniel-J-Mason Sep 14, 2023

Daniel-J-Mason Sep 14, 2023 •

edited

Loading

jakaarl left a comment

jakaarl Oct 7, 2023

jakaarl Oct 7, 2023

jakaarl Oct 7, 2023

jakaarl Oct 7, 2023

jakaarl Oct 7, 2023

jakaarl Oct 7, 2023

jakaarl Oct 7, 2023

#103 - adding pagination #104

Are you sure you want to change the base?

#103 - adding pagination #104

Conversation

pveeckhout commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel-J-Mason Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

jakaarl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel-J-Mason Sep 14, 2023 •

edited

Loading