edgedb / edgedb-js

The official TypeScript/JS client library and query builder for EdgeDB
https://edgedb.com
Apache License 2.0
513 stars 65 forks source link

EdgeQLSyntaxError: Unexpected keyword "IN" / mixing string type literals and type names is not supported #577

Closed NetOpWibby closed 1 year ago

NetOpWibby commented 1 year ago

Code The code causing the error.

(async() => {
  let databaseQuery;

  switch((query.type as Record["type"])) {
    case "A":
    case "AAAA":
    case "CNAME":
    case "DNAME":
    case "NS":
    case "PTR": {
      databaseQuery = e.select(
        e.insert(e.PlainRecord, { ...query }),
        () => ({ ...e.PlainRecord["*"] })
      );
      break;
    }

    default: {
      break;
    }
  }

  try {
    response = await databaseQuery.run(client);
    return { detail: response };
  } catch(_) {
    return { detail: response };
  }
});

Schema

Your application schema.

module default {
  scalar type ClassType extending enum<"CH", "CS", "HS", "IN">;

  abstract type BaseRecord {
    required property class -> ClassType;
    required property created -> datetime {
      default := datetime_of_transaction();
      readonly := true;
    };
    required property name -> str;
    required property ttl -> float64;
    required property type -> RecordType;
    required property updated -> datetime {
      default := datetime_of_transaction();
    };
  }

  type PlainRecord extending BaseRecord {
    required property data -> str;
  }
}

Generated EdgeQL

SELECT (INSERT default::PlainRecord {
  name := "app.beachfront",
  type := default::RecordType.A,
  class := default::ClassType.IN,
  ttl := 300
}) {
  data,
  class,
  name,
  ttl,
  type,
  created,
  updated,
  id
}

Error or desired behavior

SS 2023-04-20 at 1 23 12 PM

Versions (please complete the following information):

NetOpWibby commented 1 year ago

The mixing string type literals and type names is not supported error was achieved by replacing quotes with backticks around IN in my schema.

scotttrinh commented 1 year ago

replacing quotes with backticks around IN in my schema.

Can you detail what you mean by this? Did that error message show up during a migration, or at runtime?

Does this work:

  scalar type ClassType extending enum<CH, CS, HS, `IN`>;

—?

NetOpWibby commented 1 year ago

@scotttrinh I get No schema changes detected. when I remove quotes from the first three enum values.

scotttrinh commented 1 year ago

Meaning that you updated it to include the backtick version of IN and no migrations were required, or just updated to be <CH, CS, HS, "IN">?

I think what we're trying to get to is a quoted IN enum value, and then in your runtime code you're going to need to also add the backtick quoting by transforming like:

if (query.class === "IN") {
  query.class = "`IN`";
}

— or some similar runtime code.

and make sure the schema has the IN enum member quoted in backticks, too.

NetOpWibby commented 1 year ago

Meaning that you updated it to include the backtick version

Correct. I then tried to do something like

scalar type ClassType extending enum<"CH", "CS", "HS", "`IN`">;

but got

edgedb error: InvalidValueError: invalid input value for enum 'default::ClassType': "IN"

I'll try the backtick transform in my code.

NetOpWibby commented 1 year ago

@scotttrinh EdgeDB returned with this:

Error: Invalid value for type default::ClassType: "`IN`"
    at literalToEdgeQL (file:///~/dbschema/edgeql-js/toEdgeQL.ts:1430:15)
    at renderEdgeQL (file:///~/dbschema/edgeql-js/toEdgeQL.ts:857:12)
    at shapeToEdgeQL (file:///~/dbschema/edgeql-js/toEdgeQL.ts:1334:26)
    at renderEdgeQL (file:///~/dbschema/edgeql-js/toEdgeQL.ts:1026:10)
    at renderEdgeQL (file:///~/dbschema/edgeql-js/toEdgeQL.ts:910:31)
    at Proxy.$toEdgeQL (file:///~/dbschema/edgeql-js/toEdgeQL.ts:296:16)
    at Proxy.$queryFunc (file:///~/dbschema/edgeql-js/query.ts:28:22)
    at Object.default [as createRecord] (file:///~/src/api/component/record/crud/create.ts:148:36)
    at eventLoopTick (ext:core/01_core.js:166:11)
    at async runHttpQuery (https://deno.land/x/alpha@2023.04.10/src/common.ts:40:10)

Data supplied to EdgeDB:

{ name: "app.beachfront", type: "A", class: "`IN`", ttl: 300 }

EDIT: Using backticks only in my code returns the original error message.

jaclarke commented 1 year ago

@scotttrinh I think this is a bug in querybuilder. In the edgeql codegen it looks like we're not correctly quoting enum values that are also keywords, ie. the generated edgeql should be:

SELECT (INSERT default::PlainRecord {
  name := "app.beachfront",
  type := default::RecordType.A,
  class := default::ClassType.`IN`,
  ttl := 300
}) {
  data,
  class,
  name,
  ttl,
  type,
  created,
  updated,
  id
}
scotttrinh commented 1 year ago

@jaclarke

Quoting should be done at query time or at schema defintion time or both?

jaclarke commented 1 year ago

Just at query time I think. The schema I think is technically using the older enum syntax, but it's still valid so it's fine. (Syntax change details: https://github.com/edgedb/edgedb/issues/1833#issuecomment-700235349).

NetOpWibby commented 1 year ago

Is the Deno library gonna get updated? It's still on 1.1.0 and there was a new repo release last week.

scotttrinh commented 1 year ago

@NetOpWibby Yeah, I noticed that and opened an issue to figure that out #599 . Hope to have a new release of the library out next week that incorporates this change and does the Deno release.

NetOpWibby commented 1 year ago

I finally had a chance to test the Deno release. Recompiled the query builder and am now getting a different error:

reservedKeywords.includes is not a function

I don't think this error is coming from my project as I don't have anything called reservedKeywords in my codebase.

EDIT:

Ran deno check and got:

TS2339 [ERROR]: Property 'includes' does not exist on type 'Set<string>'.
      reservedKeywords.includes(lident);
                       ~~~~~~~~
    at file:///project/dbschema/edgeql-js/toEdgeQL.ts:1545:24

EDIT DEUX:

https://github.com/edgedb/edgedb-js/blob/464b0988121e7183a8304b37014a096ff0d01895/packages/generate/src/syntax/toEdgeQL.ts#L1580 shows reservedKeywords.has(lident). @scotttrinh do you know why the generated code would switch from has to includes? Array.from(reservedKeywords).includes should work but I feel like the original code shouldn't transform the way it did.

scotttrinh commented 1 year ago

We recently switched from an array to a set for quoting these keywords. You'll need to regenerate your query builder code from the latest verison which should properly generate a call to has instead of includes.

NetOpWibby commented 1 year ago

Guess I gotta wait for v1.2.3 for Deno to release. Idk how to use Github releases.

scotttrinh commented 1 year ago

@NetOpWibby

No, this should be working with the latest release on deno.land as well, just make sure you regenerate your edgeql-js files after updating.

NetOpWibby commented 1 year ago

It doesn't.

I was already using the latest. Noticed something interesting though...when I omit the version like so:

deno run -A --unstable https://deno.land/x/edgedb/generate.ts edgeql-js --instance nameserver --database nameserver

I get

error: Uncaught TypeError: Cannot read properties of undefined (reading 'setNextTickCallback')
core.setNextTickCallback(processTicksAndRejections);
     ^
    at https://deno.land/std@0.115.0/node/_next_tick.ts:206:6

The below command doesn't have that issue:

deno run -A --unstable https://deno.land/x/edgedb@v1.2.2/generate.ts edgeql-js --instance nameserver --database nameserver
scotttrinh commented 1 year ago

Hmmm... Looks like there is some part of the bits that copy files at build time to the edgedb-deno repo that just isn't getting updated with the current publishing setup. Will look into this.

NetOpWibby commented 1 year ago

1.3.0 works!

scotttrinh commented 1 year ago

Ah, thanks for verifying @NetOpWibby ! I meant to leave a note here that the latest release fixed the stale file bug with the Deno release, so glad it worked.