Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]: Mismatched type hints when using RDS Data API #2097

Closed
cmanou opened this issue Apr 2, 2024 · 14 comments
Closed

[BUG]: Mismatched type hints when using RDS Data API #2097

cmanou opened this issue Apr 2, 2024 · 14 comments
Labels
bug Something isn't working

Comments

@cmanou
Copy link

cmanou commented Apr 2, 2024

What version of drizzle-orm are you using?

0.30.1

What version of drizzle-kit are you using?

0.20.14

Describe the Bug

When using AwsDataApi pg driver which actually makes use of the type hints, there has been a bunch of queries that have resulted in mismatched hints. I have narrowed down the issue to the mergeQueries function in sql.ts

function mergeQueries(queries: QueryWithTypings[]): QueryWithTypings {
. This results in failed inserts and selects. In particular if the following if mergeQueries receives the following:

[
    { "sql": "(", "params": [] },
    { "sql": "default", "params": [] },
    { "sql": ", ", "params": [] },
    {
      "sql": ":1",
      "params": ["44562697-0b70-479c-8f38-f4ff9d17c537"],
      "typings": ["uuid"]
    },
    { "sql": ", ", "params": [] },
    {
      "sql": ":2",
      "params": ["value_2"]
    },
    { "sql": ", ", "params": [] },
    { "sql": ":3", "params": ["value_3"] },
    { "sql": ", ", "params": [] },
    {
      "sql": ":4",
      "params": ["{\"status\":\"enabled\"}"],
      "typings": ["json"]
    },
    { "sql": ", ", "params": [] },
    { "sql": "default", "params": [] },
    { "sql": ", ", "params": [] },
    { "sql": "default", "params": [] },
    { "sql": ", ", "params": [] },
    { "sql": "default", "params": [] },
    { "sql": ")", "params": [] }
  ]

This results in

{
    "sql": "(default, :1, :2, :3, :4, default, default, default)",
    "params": [
      "44562697-0b70-479c-8f38-f4ff9d17c537",
      "value_2",
      "value_3",
      "{\"status\":\"enabled\"}"
    ],
    "typings": ["uuid", "json"]
  }

Which has the typings indexes incorrect for when they are used later on by the dataapi session driver.

Expected behavior

Expected the result from merge queries to be similar to this, or potentially but would require more work is making it such that the typings stick with the params?

{
    "sql": "(default, :1, :2, :3, :4, default, default, default)",
    "params": [
      "44562697-0b70-479c-8f38-f4ff9d17c537",
      "value_2",
      "value_3",
      "{\"status\":\"enabled\"}"
    ],
    "typings": ["uuid",undefined, undefined, "json"]
  }

Environment & setup

No response

@cmanou cmanou added the bug Something isn't working label Apr 2, 2024
@zacronos
Copy link

zacronos commented Apr 4, 2024

I seem to have just hit this bug, which results in my "typeHint":"TIMESTAMP" hints showing up on the wrong parameters, and leaving my actual TIMSTAMP/Date parameters being treated as strings. This causes me to get this error: ERROR: column "updated_at" is of type timestamp without time zone but expression is of type character varying on what should be a valid query.

I'm using v0.30.7.

@cmanou
Copy link
Author

cmanou commented Apr 4, 2024

@AndriiSherman @AlexBlokh I would be willing to help move this forward just wondering if you have had thoughts on the approach and why typings weren't initially passed around with the actual params?

@cmanou
Copy link
Author

cmanou commented Apr 8, 2024

@dankochetov I see you were working on some data api fixes in #2119 do you have thoughts on the best approach here ?

@dankochetov
Copy link
Contributor

@cmanou could you provide an example query that triggers this behavior?

@zacronos
Copy link

I know this is a description rather than an example, but I believe it occurs any time you have a query that will generate a list of parameters where some parameters do not have type hints, followed by parameters which do have type hints. The type hints then get associated with the earlier parameters instead of the correct ones.

@cmanou
Copy link
Author

cmanou commented Apr 11, 2024

Lots off different ones this particular one was:

export const calendar = pgTable('calendars', {
  id: uuid('id')
    .primaryKey()
    .default(sql`gen_random_uuid()`),

  orgMembershipId: prefixedUlid('om_id', { prefix: 'om' })
    .notNull()

  platform: text('platform').notNull(),
  externalId: text('external_id').notNull(),
  externalData: json('external_data').notNull(),

  updatedAt: timestamp('updated_at')
    .notNull()
    .default(sql`now()`),
  createdAt: timestamp('created_at')
    .notNull()
    .default(sql`now()`),
});

export const prefixedUlid = <Prefix extends string, PrefixedUlid = `${Prefix}_${string}`>(
  name: string,
  opts: { prefix: Prefix },
) =>
  customType<{ data: PrefixedUlid; driverData: string }>({
    dataType: () => 'uuid',
    toDriver: (value) => {
      const regex = new RegExp(`^${opts.prefix}_([0-7][0-9a-hjkmnp-tv-zA-HJKMNP-TV-Z]{25})$`);
      const match = (value as string).match(regex);
      if (!match || match[1] === undefined) {
        throw new Error(`Invalid format`);
      }

      return ULIDtoUUID(match[1]);
    },
    fromDriver: (value) => {
      return `${opts.prefix}_${UUIDtoULID(value)}` as PrefixedUlid;
    },
  })(name);

  const [result] = await db
    .insert(calendars)
    .values({
      orgMembershipId,
      platform,
      externalId,
      externalData,
    })
    .returning();
    ```

But as @zacronos said this happens whenever you use queries that have a mix of type hints and non type hint variables. I believe other people have reported similar issues with selects and limits having wrong type hints with the data api and am generally convinced that the mergeQueries and how typeHints are linked to there actual values is the problem

@zacronos
Copy link

I'm pretty confident in the mergeQueries theory as well. When I hit a problem with this, based on @cmanou 's description of the underlying cause, I was able to fix my query by reordering things such that the parameters which required typeHints were all before the ones that did not.

@cmanou
Copy link
Author

cmanou commented Apr 16, 2024

@dankochetov Have you had anymore thoughts on this? Happy to help if you have a particular direction.

@cmanou
Copy link
Author

cmanou commented Apr 29, 2024

@dankochetov Just wondering if you have had anymore thoughts on this it basically makes data api un-usuable which is a shame. As I said I'm willing to work on the sulotion just wondering if you had a preference on the approach ie allow undefined or pass the hints with their values all the way through?

@cmanou
Copy link
Author

cmanou commented May 29, 2024

Okay based off the brief discussion in discord theoretically this shouldn't be happening because the typings should be filled with 'none' type but it turns out for placeholders and cases where drizzle builds internal params such as for relationships

const limitSql = limit ? sql` limit ${limit}` : undefined;

the type isn't add in these too lines
return { sql: escapeParam(paramStartIndex.value++, chunk), params: [chunk] };

return { sql: escapeParam(paramStartIndex.value++, chunk), params: [chunk] };

Updating them to have ['none'] as the types resolves the issue but feels incorrect as really we should be trying to use prepareTyping but for a placeholder the type is not really known, so although this fix would work for my use cases it does not work if people are using types such as uuid with preparedQueries

@dankochetov
Copy link
Contributor

@zacronos could you test if it works with drizzle-orm@beta?

@zacronos
Copy link

If I had a convenient query, I would love to help -- unfortunately I've entirely forgotten which query was the source of the problem for me, and we're swamped with customer onboarding at the moment.

@AndriiSherman
Copy link
Member

should be fixed in drizzle-orm@0.31.3. If you still see this issue, please reopen it

@Steve2955
Copy link

Steve2955 commented Jul 22, 2024

@AndriiSherman Thanks for jumping on this, but I'm still facing an issue with drizzle-orm@0.32.0.

I also narrowed down the issue to the mergeQueries function in sql.ts and determined the cause to be missing typings for some params. The below code fixes the issue for me:

function mergeQueries(queries: QueryWithTypings[]): QueryWithTypings {
	const result: QueryWithTypings = { sql: '', params: [] };
	for (const query of queries) {
		result.sql += query.sql;
		result.params.push(...query.params);
		if (query.typings?.length) {
			if (!result.typings) {
                                // setting an array of "none" for possible prior params 
                                // without typing resolves the issue for me:
				result.typings = new Array(result.params.length - query.params.length).fill("none");
			}
			result.typings.push(...query.typings);
		}
	}
	return result;
}

Logged query in my case:

SELECT 
    "stations"."id",
    "stations"."external_id",
    "stations"."name",
    "stations"."latitude",
    "stations"."longitude",
    "stations"."operator_id",
    "stations"."created_at",
    "stations"."updated_at",
    "stations_operator"."data" AS "operator",
    "stations_evses"."data" AS "evses"
FROM "stations"
LEFT JOIN LATERAL (
    SELECT json_build_array(
        "stations_operator"."id",
        "stations_operator"."name",
        "stations_operator"."last_imported"
    ) AS "data"
    FROM (
        SELECT * FROM "operators" "stations_operator"
        WHERE "stations_operator"."id" = "stations"."operator_id"
        LIMIT :1
    ) "stations_operator"
) "stations_operator" ON true
LEFT JOIN LATERAL (
    SELECT coalesce(
        json_agg(
            json_build_array(
                "stations_evses"."uid",
                "stations_evses"."operator_id",
                "stations_evses"."evse_id",
                "stations_evses"."station",
                "stations_evses"."updated_at",
                "stations_evses_connectors"."data"
            )
        ), '[]'::json
    ) AS "data"
    FROM "evses" "stations_evses"
    LEFT JOIN LATERAL (
        SELECT coalesce(
            json_agg(
                json_build_array(
                    "stations_evses_connectors"."id",
                    "stations_evses_connectors"."operator_id",
                    "stations_evses_connectors"."evse_uid",
                    "stations_evses_connectors"."standard",
                    "stations_evses_connectors"."max_voltage",
                    "stations_evses_connectors"."max_amperage",
                    "stations_evses_connectors"."updated_at"
                )
            ), '[]'::json
        ) AS "data"
        FROM "connectors" "stations_evses_connectors"
        WHERE (
            "stations_evses_connectors"."evse_uid" = "stations_evses"."uid"
            AND "stations_evses_connectors"."operator_id" = "stations_evses"."operator_id"
        )
    ) "stations_evses_connectors" ON true
    WHERE "stations_evses"."station" = "stations"."id"
) "stations_evses" ON true
WHERE (
    "stations"."external_id" = :2
    AND "stations"."operator_id" = :3
    AND "stations"."updated_at" < :4
)
LIMIT :5;

-- params: [
-- {
--     "name": "1",
--     "value": {
--         "longValue": 1
--     }
-- },
-- {
--     "name": "2",
--     "value": {
--         "stringValue": "S104"
--     },
--     "typeHint": "UUID" --> should be typing of param "3"
-- },
-- {
--     "name": "3",
--     "value": {
--         "stringValue": "1ca49824-d2cb-4bc1-96de-af8b3d68be12"
--     },
--     "typeHint": "TIMESTAMP" --> should be typing of param "4"
-- },
-- {
--     "name": "4",
--     "value": {
--         "stringValue": "2024-07-22T08:39:59.943Z"
--     }
-- },
-- {
--     "name": "5",
--     "value": {
--         "longValue": 1
--     }
-- }
-- ]
const data = await db.query.stations.findFirst({
    where: (stations) => {
      return and(
        eq(stations.externalId, ids.location_id),
        eq(stations.operatorId, ids.operator_id),
        lt(stations.updatedAt, new Date()), // Only return if the station is not updated
      );
    },
    with: {
      operator: true,
      evses: {
        with: {
          connectors: true,
        },
      },
    },
  });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants