[mds-ingest-service] Add pagination and order support to getEvents via ingest #643

cjlynch12 · 2021-07-01T14:12:17Z

📚 Purpose

This PR adds pagination and order support for the existing getEvents() service method in mds-ingest-service.

👌 Resolves:

📦 Impacts:

mds-ingest-service

✅ PR Checklist

cjlynch12 · 2021-07-01T14:21:34Z

packages/mds-ingest-service/repository/entities/event-entity.ts

@@ -42,7 +42,7 @@ export class EventEntity extends IdentityColumn(RecordedColumn(class {})) implem
  provider_id: EventEntityModel['provider_id']

  @Column('bigint', { transformer: BigintTransformer, primary: true })
-  timestamp: EventEntityModel['timestamp']
+  timestamp: Timestamp


Strange type behavior here with the typeorm-cursor-pagination lib.
When this is left as EventEntityModel['timestamp'], and you pass limit to the buildPaginator(...) function, it results in the following error:

unknown type in cursor: [object]1625149089973

Which corresponds to the time_range: {begin} parameter.

Interestingly, this is only an issue when limit is passed. If the type declaration here is left as EventEntityModel['timestamp'] and limit isn't provided, it works fine. As soon as limit is provided, it breaks.

Changing this to Timestamp directly here fixes the issue in both cases.

This doesn't have any issues afaict, all previous tests are still passing with this change. 🤷

Yep. We’ve seen this before.

We've recently stopped defining entity properties in terms of Entity or (especially) Domain model properties and use the base types instead. Some libraries that use metadata reflection have issues tracing the aliases to the base type. In recent repositories, you will see a pattern something like:

export class SomeEntity { @Column(...) entity_id: UUID @Column(...) timestamp: Timestamp } /* Create an alias to keep consistent with other services */ export SomeEntityModel = SomeEntity

cjlynch12 · 2021-07-01T14:23:31Z

packages/mds-ingest-service/repository/index.ts

@@ -199,14 +219,61 @@ class IngestReadWriteRepository extends ReadWriteRepository {
        query.andWhere('events.provider_id = ANY(:provider_ids)', { provider_ids })
      }

-      const entities = await query.limit(limit).getMany()
+      // Use query instead of paginator to manage order if using a joined field


couldn't figure out how to pass an order column string for a joined field to buildPaginator without it throwing an error, if there's a better way to handle this field happy to change it.

nessur

Nice work!

terloon · 2021-07-01T15:00:45Z

packages/mds-ingest-service/@types/index.ts

@@ -70,6 +70,19 @@ export type TimeRange = {
  start: Timestamp
  end: Timestamp
 }
+
+export const GetVehicleEventsOrderColumn = <const>['timestamp', 'vehicle_id', 'provider_id', 'vehicle_state']


Just curious as I've seen this in other places as I learn typescript. Is it common to name the value and type name the same? I've seen it where the value name is descriptive and would name it like "GetVehicleEventOrderColumns" (with an s to describe these are all the columns) and the type is then without the "s". Also, is it preferred to use vs "as const"? Are they the same?

I've seen instances of both

export const FooTypes = <const>['bar', 'baz'] export type FooType = typeof FooTypes[number]

and

export const FooType = <const>['bar', 'baz'] export type FooType = typeof FooType[number]

In various places in the code base. I think the latter is the preferred.

My assumption is that the latter just reduces the overhead of knowing which type to choose (FooTypes vs FooTypes) since TS will interpret it as the type declaration. (@drtyh2o feel free to tell me I'm off base here 😄 ).

As for
<const> foo vs foo as const, they are equivalent except in a .tsx file.

mdurling

There are probably opportunities for creating some cursor utilities as there seems to be a lot of similarities between this backend and the trips backend, but this LGTM.

mdurling · 2021-07-01T14:27:55Z

packages/mds-ingest-service/repository/entities/event-entity.ts

@@ -42,7 +42,7 @@ export class EventEntity extends IdentityColumn(RecordedColumn(class {})) implem
  provider_id: EventEntityModel['provider_id']

  @Column('bigint', { transformer: BigintTransformer, primary: true })
-  timestamp: EventEntityModel['timestamp']
+  timestamp: Timestamp


Yep. We’ve seen this before.

mdurling · 2021-07-01T16:07:49Z

packages/mds-ingest-service/repository/entities/event-entity.ts

@@ -42,7 +42,7 @@ export class EventEntity extends IdentityColumn(RecordedColumn(class {})) implem
  provider_id: EventEntityModel['provider_id']

  @Column('bigint', { transformer: BigintTransformer, primary: true })
-  timestamp: EventEntityModel['timestamp']
+  timestamp: Timestamp


We've recently stopped defining entity properties in terms of Entity or (especially) Domain model properties and use the base types instead. Some libraries that use metadata reflection have issues tracing the aliases to the base type. In recent repositories, you will see a pattern something like:

export class SomeEntity { @Column(...) entity_id: UUID @Column(...) timestamp: Timestamp } /* Create an alias to keep consistent with other services */ export SomeEntityModel = SomeEntity

mdurling · 2021-07-01T16:11:54Z

packages/mds-ingest-service/repository/index.ts

-      const entities = await query.limit(limit).getMany()
+      // Use query instead of paginator to manage order if using a joined field
+      if (order && order.column === 'vehicle_id') {
+        query.orderBy('devices.vehicle_id', order.direction)


I'm unsure how the query.orderBy and paginationKeys interact, but I assume the pagination keys will get appended to the orderBy. Is timestamp always the secondary sort column?

Good call out, I took a look at this and it turns out that paginationKeys always override anything added to the query with orderBy or addOrderBy. If you remove paginationKeys param entirely, it just uses id (the default).

There seems to be an open issue for this (can't sort on relational tables / override paginationKeys) - and the maintainer has ack'd it recently.
benjamin658/typeorm-cursor-pagination#4

So for now, removed the ability to sort by vehicle_id. Will follow up with the open issue. Not worth sinking a bunch of time for this one sort-able field atm.

That being said, sorting on joined tables will most likely come up in the future at some point - so hopefully this gets addressed in typeorm-cursor-pagination shortly.

PS - updated tests since apparently they were false positives for sorting on vehicle_id.

…order

…events

cjlynch12 added 3 commits June 30, 2021 18:57

Add cursor based pagination support to ingest events service

8235e29

Add order support to ingest events service

ac998ae

Revert type change used during testing

3e0d597

cjlynch12 commented Jul 1, 2021

View reviewed changes

cjlynch12 requested review from avatarneil, BigIdeas, mdurling, nessur and terloon July 1, 2021 14:24

nessur approved these changes Jul 1, 2021

View reviewed changes

terloon reviewed Jul 1, 2021

View reviewed changes

mdurling approved these changes Jul 1, 2021

View reviewed changes

mdurling reviewed Jul 1, 2021

View reviewed changes

cjlynch12 added 2 commits July 1, 2021 13:48

Remove vehicle_id sort option for now, update tests to actually test …

f89ae4a

…order

Merge branch 'develop' into carter/feature/add-sorting-paging-ingest-…

e63f378

…events

cjlynch12 added the bulldozer-merge label Jul 2, 2021

lacuna-tech-bulldozer bot merged commit f4500c6 into develop Jul 2, 2021

lacuna-tech-bulldozer bot deleted the carter/feature/add-sorting-paging-ingest-events branch July 2, 2021 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mds-ingest-service] Add pagination and order support to getEvents via ingest #643

[mds-ingest-service] Add pagination and order support to getEvents via ingest #643

cjlynch12 commented Jul 1, 2021

cjlynch12 Jul 1, 2021 •

edited

Loading

mdurling Jul 1, 2021

mdurling Jul 1, 2021

cjlynch12 Jul 1, 2021

nessur left a comment

terloon Jul 1, 2021

cjlynch12 Jul 1, 2021 •

edited

Loading

mdurling left a comment

mdurling Jul 1, 2021

mdurling Jul 1, 2021

mdurling Jul 1, 2021

cjlynch12 Jul 1, 2021 •

edited

Loading

[mds-ingest-service] Add pagination and order support to getEvents via ingest #643

[mds-ingest-service] Add pagination and order support to getEvents via ingest #643

Conversation

cjlynch12 commented Jul 1, 2021

📚 Purpose

👌 Resolves:

📦 Impacts:

✅ PR Checklist

cjlynch12 Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

mdurling Jul 1, 2021

Choose a reason for hiding this comment

mdurling Jul 1, 2021

Choose a reason for hiding this comment

cjlynch12 Jul 1, 2021

Choose a reason for hiding this comment

nessur left a comment

Choose a reason for hiding this comment

terloon Jul 1, 2021

Choose a reason for hiding this comment

cjlynch12 Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

mdurling left a comment

Choose a reason for hiding this comment

mdurling Jul 1, 2021

Choose a reason for hiding this comment

mdurling Jul 1, 2021

Choose a reason for hiding this comment

mdurling Jul 1, 2021

Choose a reason for hiding this comment

cjlynch12 Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

cjlynch12 Jul 1, 2021 •

edited

Loading

cjlynch12 Jul 1, 2021 •

edited

Loading

cjlynch12 Jul 1, 2021 •

edited

Loading