DPLT-1136: Implement update, upsert, and delete on context.db

[darunrs]

NOTE: PR targeting my active PR #188 for now to show accurate diff. Once that PR is merged, I'll switch this PR back to main (Or open a new one) and do a push to fix the commit history and reopen for review. This is just to be available to look at in the mean time.

PR 188 has been merged and I have rebased and force pushed the history to this PR. There are no conflicts with main.

Adding functionality for update, upsert, delete. Added relevant tests as well as performed simple integration tests using existing tests. No problems found there. Will do a full test in dev after push as well by updating a forked social_feed indexer to use context.db exclusively.

[roshaans]

Good job, lgtm overall. Just left some comments on testing and conventions.

[roshaans]

Seems odd that we are awaiting a variable here. How about calling await this.initialize() and then inside of the initialize() method, you can change it to return early if it is already initialized.

You can also change the function signature of initialize() to return a Boolean rather than a void to make it more clearer.

[darunrs]

The reason I went with a variable is because I want the initialize function to be called in the constructor. However, I cannot await a function there. So, instead, I call it while setting the result to a variable. That way, I can call the function and not await in the constructor. Then, when a method is called, it just checks to see if the variable has been set. This was the most common pattern I could find for calling an async method in the constructor. It gives an error if I try to call the function as is. The return type of Promise is because we don't actually care about the result of the function, just that it finished and set the variable to something.

[morgsmccauley]

I found another way to work around async constructors:

Instead of creating the class via the constructor itself, we can create static method to do it. This method could be async, and enforce all the additional constraints that weren't possible in a normal constructor. We can then make the contructor private so that you have to use the new method, e.g.

export default class DmlHandler {
  private constructor (
    private readonly account: string,
    private readonly hasuraClient: HasuraClient,
    private readonly pgClient: PgClient,
  ) {}

  static async new (
    account: string,
    hasuraClient: HasuraClient = new HasuraClient(),
    PgClient = PgClientModule,
  ): Promise<DmlHandler> {
    const connectionParameters = await hasuraClient.getDbConnectionParameters(account);
    const pgClient = new PgClient({
      user: connectionParameters.username,
      password: connectionParameters.password,
      host: process.env.PGHOST,
      port: Number(connectionParameters.port),
      database: connectionParameters.database,
    });

    return new DmlHanlder(account, hasuraClient, pgClient);
  }
}

We no longer need the await this.initialize since we are passing in an already initialised client. We would create the class it like so:

const dmlHandler = await DmlHandler.new('morgs.near');

Now that construction of DmlHandler is async, we would have to change every class which uses it to also follow this pattern. This probably isn't something to commit to right now, but should be something we work towards as it provides a lot more flexibility.

[darunrs]

That's neat! I'll give it a shot. Also updated the PR name.

[roshaans]

I think it also makes sense to cover the non-happy path scenarios such as testing to see if the update, delete, upsert methods fail as expected when garbage, malformed, or nonexistent table or field values are passed in to them.

[darunrs]

So we actually do not support failure when invalid input is provided as we wouldn't know that unless we do one of two things:

1. Run the query itself.
2. Know what the input object structure should look like.

1 is not helpful and 2, is a work in progress. We need to do the type generation on the schema to get table object types. We can use that to ensure good information is passed in. Unfortunately for now, we don't have that. The best I could do was use typescript strong typing to ensure the inputs sort of look normal.

[morgsmccauley]

Can you please add the jira ticket to the PR title i.e. DPLT-XXXX feat: Implement...

[morgsmccauley]

I found another way to work around async constructors:

Instead of creating the class via the constructor itself, we can create static method to do it. This method could be async, and enforce all the additional constraints that weren't possible in a normal constructor. We can then make the contructor private so that you have to use the new method, e.g.

export default class DmlHandler {
  private constructor (
    private readonly account: string,
    private readonly hasuraClient: HasuraClient,
    private readonly pgClient: PgClient,
  ) {}

  static async new (
    account: string,
    hasuraClient: HasuraClient = new HasuraClient(),
    PgClient = PgClientModule,
  ): Promise<DmlHandler> {
    const connectionParameters = await hasuraClient.getDbConnectionParameters(account);
    const pgClient = new PgClient({
      user: connectionParameters.username,
      password: connectionParameters.password,
      host: process.env.PGHOST,
      port: Number(connectionParameters.port),
      database: connectionParameters.database,
    });

    return new DmlHanlder(account, hasuraClient, pgClient);
  }
}

We no longer need the await this.initialize since we are passing in an already initialised client. We would create the class it like so:

const dmlHandler = await DmlHandler.new('morgs.near');

Now that construction of DmlHandler is async, we would have to change every class which uses it to also follow this pattern. This probably isn't something to commit to right now, but should be something we work towards as it provides a lot more flexibility.

[morgsmccauley]

Why can this be null?

[darunrs]

I was under the impression that I could not use ?? on an uninitialized variable. I tested that in some online compilers and got an error. However, I just removed it now and it seems to be working. So, I will make it let dmlHandler: DmlHandler;.

[morgsmccauley]

great job