Add postgres tap bugfixes & integration tests #229

sherifnada · 2020-09-11T16:59:17Z

What

Adds integration tests for Postgres tap and fixes some related issues. The main issues were around how we encoded Singer messages:

RECORD messages didn't have a version field
schema field on SCHEMA messages was the wrong type
We didn't support the undocumented ACTIVATE_VERSION messages.

Reading order

SingerMessage.json
SingerPostgresSourceTap
everything else

…ostgres-tap-integration-tests

sherifnada · 2020-09-11T17:10:02Z

dataline-tests/src/acceptanceTests/java/io/dataline/tests/acceptance/AcceptanceTests.java

@@ -100,14 +101,6 @@ public static void tearDown() {
    TARGET_PSQL.stop();
  }

-  private static void runSqlScript(MountableFile file, PostgreSQLContainer db)


moved to a common helper

sherifnada · 2020-09-11T17:13:47Z

dataline-db/src/main/java/io/dataline/db/PostgreSQLContainerHelper.java

+import org.testcontainers.containers.PostgreSQLContainer;
+import org.testcontainers.utility.MountableFile;
+
+public class PostgreSQLContainerHelper {


refactored this from other tests since we need to run this in many tests

for now I prefer that each test does that until we figure out a way to expose is as a helper

it sounds like the issue that it is coupled with the db module, in which case this would be better exposed in a separate test utils package?

That's correct

Moved this to a separate module

sherifnada · 2020-09-11T17:23:49Z

dataline-singer/src/main/resources/json/SingerMessage.json

  "required": ["type"],
  "properties": {
    "type": {
      "description": "record, schema, and state message: used to determine record type. used in all singer messages.",
      "type": "string",
-      "enum": ["RECORD", "SCHEMA", "STATE"]
+      "enum": ["RECORD", "SCHEMA", "STATE", "ACTIVATE_VERSION"]


some context around ACTIVATE_VERSION:

it seems to have only the version field

confused person on singer slack

some comments in the source code of a random tap explaining it: link

i opened an issue in Singer asking to document it link

Can we hold off on implementing it? or does it break smth to not have it?

I don't think it should break anything since this isn't an officially supported field yet.

woof. yeah. i'd rather just ignore this type of message then add it into "our" protocol.

sherifnada · 2020-09-11T17:28:40Z

Looks like this broke tests elsewhere in the repo, fixing

michel-tricot · 2020-09-11T17:24:50Z

dataline-db/build.gradle

@@ -7,6 +7,7 @@ dependencies {
    api 'org.jooq:jooq-meta:3.13.4'
    api 'org.jooq:jooq:3.13.4'
    api 'org.postgresql:postgresql:42.2.16'
+    api 'org.testcontainers:postgresql:1.14.3'


I disagree with that. We shouldn't have testcontainer be a dep for dataline-db. It should remain a testImplementation

michel-tricot · 2020-09-11T17:25:33Z

dataline-db/src/main/java/io/dataline/db/PostgreSQLContainerHelper.java

+import org.testcontainers.containers.PostgreSQLContainer;
+import org.testcontainers.utility.MountableFile;
+
+public class PostgreSQLContainerHelper {


for now I prefer that each test does that until we figure out a way to expose is as a helper

michel-tricot · 2020-09-11T17:26:18Z

dataline-integrations/singer/postgres/source/build.gradle

+    integrationTestImplementation project(':dataline-singer')
+}
+
+import com.bmuschko.gradle.docker.tasks.image.DockerBuildImage


don't forget to add the imagename task (see bigquery gradle)

michel-tricot · 2020-09-11T17:27:34Z

...r/postgres/source/src/test-integration/resources/simple_schema_update_expected_messages.json

+      "stream": "id_and_name",
+      "record": {
+        "id": 6,
+        "name": "sushi"


not the edible kind I'm afraid 😅

michel-tricot · 2020-09-11T17:27:51Z

dataline-singer/src/main/resources/json/SingerMessage.json

  "required": ["type"],
  "properties": {
    "type": {
      "description": "record, schema, and state message: used to determine record type. used in all singer messages.",
      "type": "string",
-      "enum": ["RECORD", "SCHEMA", "STATE"]
+      "enum": ["RECORD", "SCHEMA", "STATE", "ACTIVATE_VERSION"]


what is that?

See comment on the file -- it's an undocumented message type in singer that is widely used by DBs apparently

michel-tricot · 2020-09-11T18:47:40Z

dataline-integrations/singer/postgres/source/build.gradle

+        exceptionFormat "full"
+    }
+    mustRunAfter test
+}


you're misssing the deps to buildImage here

michel-tricot · 2020-09-11T18:50:01Z

dataline-integrations/singer/postgres/source/.dockerignore

@@ -0,0 +1,2 @@
+build


Why not make in a allow list instead:

* !Dockefile !...

Nice, wasn't familiar with that pattern. Done.

cgardens · 2020-09-11T18:40:05Z

dataline-singer/src/main/resources/json/SingerCatalog.json

@@ -69,7 +69,7 @@
    },
    "SingerColumn": {
      "type": "object",
-      "additionalProperties": false,
+      "additionalProperties": true,


@cgardens I actually removed this since it is not needed; see comment below

cgardens · 2020-09-11T18:42:16Z

dataline-singer/src/main/resources/json/SingerMessage.json

@@ -43,8 +44,12 @@
        "type": "string"
      }
    },
+    "version": {


this isn't part of the singer spec? would it make sense to do what you did in catalog and just allow additional fields? my intuition is that we only explicitly add the field here is if it something that both source and destination need to know about.

This is a compromise because JsonSchema2Pojo does not correctly generate annotations which indicate required fields (e.g by adding the @NotNull annotation to the field). As a result, the jackson deserializer does not validate that required fields are present. This means that if additionalProperties=true, Jackson will deserialize pretty much any valid JSON object as a SingerMessage without validating required properties.

The only mitigating factor is that when setting additionalProperties to false, we effectively "whitelist" which fields are allowed to be present in the string that we deserialize to JsonMessage. Definitely a shitty situation, but this seems like the less bad tradeoff to make unless I'm missing that there is a JsonSchema validator that is easy to use somewhere.

Related issues:

requires should generate "required='true'" attribute in Jackson @JsonProperty annotation joelittlejohn/jsonschema2pojo#429

Jsr 305 and required joelittlejohn/jsonschema2pojo#1128

cgardens · 2020-09-11T18:44:30Z

dataline-workers/src/main/java/io/dataline/workers/singer/SingerTapFactory.java

-                          final ProcessBuilderFactory pbf,
-                          final StreamFactory streamFactory,
-                          final SingerDiscoverSchemaWorker discoverSchemaWorker) {
+  @VisibleForTesting


why not just leave this public? nbd either way. just curious.

very small thing, but it was confusing as an external consumer of this package (I was using it in integration tests) if I was supposed to pass in the stream factory. Seems like this is only useful for tests, so I removed it so that the confusion doesn't happen.

cgardens · 2020-09-11T18:55:07Z

dataline-singer/src/main/resources/json/SingerMessage.json

  "required": ["type"],
  "properties": {
    "type": {
      "description": "record, schema, and state message: used to determine record type. used in all singer messages.",
      "type": "string",
-      "enum": ["RECORD", "SCHEMA", "STATE"]
+      "enum": ["RECORD", "SCHEMA", "STATE", "ACTIVATE_VERSION"]


woof. yeah. i'd rather just ignore this type of message then add it into "our" protocol.

cgardens

blocking on the two questions in SingerMessage. after that g2g.

cgardens · 2020-09-11T18:57:09Z

dataline-singer/src/main/resources/json/SingerMessage.json

      "type": "object",
      "existingJavaType": "com.fasterxml.jackson.databind.JsonNode"
    },
    "schema": {
      "description": "schema message: the schema",
-      "$ref": "SingerCatalog.json#/definitions/SingerColumnMap"
+      "type": "object",


i thought the schema is a well defined object in the singer spec that matches the schema of the schema in the catalog jsonnot just a json blob. am i wrong about this?

the schema field is a JsonSchema description. So we can either find a $ref definition for a JsonSchema object, specify it exactly ourselves, or call it a JsonNode. I think given that we do not introspect into the schema messages during sync, the most fault tolerant thing is to treat it as a black box. Should we want to introspect into it, we should specify it more closely. WDYT?

jrhizor · 2020-09-11T19:42:40Z

...io/dataline/integrations/io/dataline/integration_tests/sources/SingerPostgresSourceTest.java

+        continue;
+      }
+
+      // We might not want to check that all fields are the same to pass a test e.g: time_extracted isn't


Could we just overwrite certain fields like time_extracted and then just compare the whole object

jrhizor · 2020-09-11T19:45:16Z

...io/dataline/integrations/io/dataline/integration_tests/sources/SingerPostgresSourceTest.java

+        .withStandardSync(syncConfig)
+        .withSourceConnectionImplementation(sourceImpl);
+
+    Stream<SingerMessage> singerMessageStream = singerTapFactory.create(tapConfig, jobRoot);


I thought for the integration-specific tests we wanted to just test that the config -> output works properly, and not test the tap factory/dataline runner infrastructure?

I think once we have comprehensive knowledge of all the things that can come out of a singer tap at compile-time, I agree with that approach. But initially (and maybe for a while), it's going to be hard to know that. Given this difficulty, IMO the better tradeoff to make is to have some duplication of test functionality for the benefit of providing higher confidence that all the machinery works well together, rather than the other way around.

For example: tap-postgres produced a SCHEMA message which failed our JsonSchema encoding of singer messages. It also produced a RECORD message which also failed our JsonSchema encoding for a different reason. And it exposed that there is a different type of message, ACTIVATE_VERSION that is defacto used by many taps, but isn't documented in the spec because it's still experimental. If we didn't test the TapFactory in this test and used a processbuilderfactory etc.. we would have only found out those issues when running the full application.

So I think for now the approach should be for now: port any such findings into unit tests where applicable (e.g: add those funky records types into SingerTapFactoryTest) and continue to use wider machinery than is necessary in integration tests until we develop enough confidence that our unit tests are comprehensive enough that we can use more barebones structs in our integration tests (which potentially will come once integrations are fully encapsulated in docker images e.g: when stdout of a docker image outputs dataline data models instead of singer data models that we then convert in the application).

WDYT?

Talked offline. Sounds good.

cgardens · 2020-09-11T21:31:55Z

discussed offline and we're all on same page now!

…ostgres-tap-integration-tests

sherifnada added 7 commits September 9, 2020 21:05

save

a8773c4

save

b76062c

save

20322ff

that dont make no sense

33b9143

Merge branch 'master' of github.com:datalineio/dataline into sherif/p…

6d65e32

…ostgres-tap-integration-tests

save

373c730

dep

b51e2a9

sherifnada commented Sep 11, 2020

View reviewed changes

sherifnada marked this pull request as ready for review September 11, 2020 17:14

please the style baron

f0472fe

sherifnada commented Sep 11, 2020

View reviewed changes

michel-tricot requested changes Sep 11, 2020

View reviewed changes

sherifnada added 4 commits September 11, 2020 10:50

move test helper to test-utils package

af79661

refactors

93b4d6c

add imagename task

8508dd7

fix tests

4d5f49f

sherifnada requested review from michel-tricot and jrhizor September 11, 2020 18:38

michel-tricot approved these changes Sep 11, 2020

View reviewed changes

deps

e3a7fc9

cgardens reviewed Sep 11, 2020

View reviewed changes

allow list instead

696dcfd

cgardens requested changes Sep 11, 2020

View reviewed changes

singer catalog additionalproperties=false

17476d4

jrhizor reviewed Sep 11, 2020

View reviewed changes

sherifnada added 4 commits September 11, 2020 13:52

use singertableschema

31be188

comment out failing tests

234d76b

comment out tests

d6c853d

singertableschema

ccfcc00

cgardens approved these changes Sep 11, 2020

View reviewed changes

remove value field

80f7a8b

jrhizor approved these changes Sep 11, 2020

View reviewed changes

sherifnada added 3 commits September 11, 2020 15:22

better comparison

ab292c6

don't include jsr305

a14923c

Merge branch 'master' of github.com:datalineio/dataline into sherif/p…

b71a640

…ostgres-tap-integration-tests

sherifnada changed the title ~~Add postgres tap integration tests~~ Add postgres tap bugfixes & integration tests Sep 11, 2020

sherifnada merged commit 0db3012 into master Sep 11, 2020

sherifnada deleted the sherif/postgres-tap-integration-tests branch September 11, 2020 22:32

cgardens mentioned this pull request Sep 11, 2020

Filter invalid Singer Messages in the worker #236

Merged

Add postgres tap bugfixes & integration tests #229

Add postgres tap bugfixes & integration tests #229

Uh oh!

Conversation

sherifnada commented Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Reading order

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sherifnada Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sherifnada commented Sep 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgardens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sherifnada commented Sep 11, 2020 •

edited

Loading

sherifnada Sep 11, 2020 •

edited

Loading