Skip to content
This repository was archived by the owner on Oct 8, 2024. It is now read-only.

add support for postgresql driver in function sdk #98

Closed
wants to merge 1 commit into from

Conversation

mangalaman93
Copy link
Member

@mangalaman93 mangalaman93 commented Jun 6, 2024

Completes HYP-1323
Also completes HYP-1324

@mangalaman93 mangalaman93 changed the title Add support for postgresql host driver with example add support for postgresql driver in function sdk Jun 6, 2024
@mangalaman93 mangalaman93 marked this pull request as ready for review June 15, 2024 13:34
@mangalaman93 mangalaman93 requested a review from a team as a code owner June 15, 2024 13:34
@mangalaman93 mangalaman93 force-pushed the aman/data branch 2 times, most recently from 769a5c4 to b334c8a Compare June 19, 2024 15:48
Comment on lines 25 to 33
postgresql.query("neon", query, [
name,
age.toString(),
balance.toString(),
privkey.toString(),
new Date(Date.now()).toUTCString(),
male.toString(),
"(" + lat.toString() + "," + lon.toString() + ")",
]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't great. The user shouldn't have to turn everything into a string.

It also means that we're passing all parameters as strings into Postgres, such as:

["John","42,123.45","[1,2,3,4]","2024-06-19T23:59:59.999Z","true","(123.45,-67.89)"]

The existing QueryVariables class could be re-purposed here, which would turn this code into something like:

  const params = new QueryVariables();
  params.set("$1", name);
  params.set("$2", age);
  params.set("$3", balance);
  params.set("$4", privkey);
  params.set("$5", new Date(Date.now()));
  params.set("$6", male);
  params.set("$7", `(${lat},${lon})`);

  postgresql.query("neon", query, params);

However, since postgres only use positional parameters (unlike SQL Server and others that use named parameters), we should make a new class in our library:

export class QueryParameters {
  private data: string[] = [];

  public add<T>(value: T): void {
    this.data.push(JSON.stringify(value));
  }

  public toJSON(): string {
    return `[${this.data.join(",")}]`;
  }
}

Then the usage can be:

  const params = new QueryParameters();
  params.add(name);
  params.add(age);
  params.add(balance);
  params.add(privkey);
  params.add(new Date(Date.now()));
  params.add(male);
  params.add(`(${lat},${lon})`);

  postgresql.query("neon", query, params);

The resulting JSON array would then not contain only strings, but for example:

["John",42,123.45,[1,2,3,4],"2024-06-19T23:59:59.999Z",true,"(123.45,-67.89)"]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you could try passing the lat/lon as an array - I'm not sure how Postges needs it though.

params.add(<f32[]>[lat, lon]);

name: string,
age: i32,
balance: f64,
privkey: i8[],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
privkey: i8[],
privkey: u8[],

Or:

Suggested change
privkey: i8[],
privkey: Uint8Array,

Or:

Suggested change
privkey: i8[],
privkey: ArrayBuffer,

Ideally the runtime should support all three (we should test that), but either way it would be unsigned.

Comment on lines 26 to 35
const input: pgInput = {
query: query,
params: params,
};

const respStr = host.databaseQuery(
datasourceName,
datasourceType,
JSON.stringify<pgInput>(input),
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets pass query and params as two separate variable to the host function. That will reduce some JSON work for the query, and will make it easier to use the QueryParameters object mentioned in the comment in the example.

Suggested change
const input: pgInput = {
query: query,
params: params,
};
const respStr = host.databaseQuery(
datasourceName,
datasourceType,
JSON.stringify<pgInput>(input),
);
const respStr = host.databaseQuery(
datasourceName,
datasourceType,
query,
params.toJSON()
);

Comment on lines 7 to 16
export function getPerson(name: string): string {
const query = "select * from people where name = $1";
return postgresql.query("neon", query, [name]);
}
Copy link
Member

@mattjohnsonpint mattjohnsonpint Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example is to get a person, but instead we get a string - which the user would have to decide how to parse. Since we're the ones in control over how the API returns data, we should return it already parsed to the format that the user is expecting. In other words, the user should not have to think too much about JSON to interpret the results.

To achieve that, the user can pass in an object to be populated as a generic type parameter. This pattern is already being used on the GraphQL example.

That also gives the user an opportunity to see how to use the results in their own application logic.

In other words:

Suggested change
export function getPerson(name: string): string {
const query = "select * from people where name = $1";
return postgresql.query("neon", query, [name]);
}
export function getPerson(name: string): Person {
const query = "select * from people where name = $1";
const people = postgresql.query<Person>("neon", query, [name]); // or params object, etc.
if (people.length == 0) {
throw new Error(`Person not found named ${name}.`);
}
if (people.length > 1) {
throw new Error(`More than one person found named ${name}.`);
}
return people[0];
}

Comment on lines 3 to 8
export function queryPeople(): string {
return postgresql.query("neon", "select * from people", []);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function queryPeople(): string {
return postgresql.query("neon", "select * from people", []);
}
export function queryPeople(): Person[] {
return postgresql.query<Person>("neon", "select * from people");
}

(see feedback on getPerson example)

Comment on lines 19 to 23
static query(
datasourceName: string,
query: string,
params: string[],
): string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static query(
datasourceName: string,
query: string,
params: string[],
): string {
static query<TData>(
datasourceName: string,
query: string,
params: QueryParameters | null = null,
): TData[] {

see feedback on the examples.

@mattjohnsonpint mattjohnsonpint marked this pull request as draft June 21, 2024 23:22
Copy link

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@typescript-eslint/eslint-plugin@7.13.0 None +6 4.55 MB jameshenry
npm/@typescript-eslint/parser@7.13.0 None +4 1.39 MB jameshenry

View full report↗︎

@mattjohnsonpint
Copy link
Member

Updated by #119 (which includes much of this)
Thanks.

@mattjohnsonpint mattjohnsonpint deleted the aman/data branch July 24, 2024 16:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants