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

Add support for enums and doc comments #551

Merged
merged 2 commits into from
Nov 8, 2021

Conversation

raycar5
Copy link
Contributor

@raycar5 raycar5 commented Oct 21, 2021

This PR adds support for enums as well as using doc comments on both entities and fields.

A few catches:

  • Enums don't work inside JSON fields.
  • [EnumName!]! gets transformed into [EnumName]! at the postgraphile layer and I haven't found a way around it.

In it's current state it's good enough for our use-case.

// "arrow-body-style": "error",
complexity: 'error',
complexity: ['error', 21],
Copy link
Contributor Author

@raycar5 raycar5 Oct 21, 2021

Choose a reason for hiding this comment

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

The previous limit was 20, if you remove it and run the tests I think you'll understand why I simply increased it by 1

@ianhe8x
Copy link
Collaborator

ianhe8x commented Oct 21, 2021

Thanks for contributing, we will play with it locally and come back to you.

@jiqiang90
Copy link
Contributor

Hi @raycar5 thank you for the contribution. I'm start work on testing this feature, can you provide a simple subquery example with generated enum, demonstrate how to use the enum in the mapping function etc? Thank you

@raycar5
Copy link
Contributor Author

raycar5 commented Oct 25, 2021

Schema:

enum TestEnum {
  Foo
  Bar
}
"""
Description
"""
type TestEntity @entity {
  """
  Field description
  """
  id: ID!
  """
  Field description
  """
  field: TestEnum!
}

Mapping function:

export async function handleEvent(event: SubstrateEvent): Promise<void> {
    const blockId = block.block.header.number.toNumber();
    const eventIdx = event.idx;
    
    await TestEntity.create({
       id: `${blockId}/${eventIdx}`,
       field: blockId % 2 === 0 ? TestEnum.Foo : TestEnum.Bar,
    }).save();
}

Query:

query {
  testEntities(filter: { field: { equalTo: Foo } }) {
    nodes {
      id
      field
    }
  }
}

@jiqiang90
Copy link
Contributor

jiqiang90 commented Oct 26, 2021

@raycar5 Hi I got this error when it try to create the enum for a new project.

-10-26T01:24:32.423Z <db> DEBUG Executing (default): DROP TYPE IF EXISTS custom_enum_2; CREATE TYPE custom_enum_2 as ENUM ('Foo','Bar'); 
2021-10-26T01:24:32.429Z <store> ERROR Having a problem when syncing schema 
SequelizeDatabaseError: cannot drop type custom_enum_2 because other objects depend on it
    at Query.formatError (/Users/jiatwork/subql2/subql/node_modules/sequelize/lib/dialects/postgres/query.js:386:16)
    at Query.run (/Users/jiatwork/subql2/subql/node_modules/sequelize/lib/dialects/postgres/query.js:87:18)

I think what happen here is each time you are trying to remove the enum have the same name, however other projects in the same database maybe also rely on it.
https://github.com/PolymathNetwork/subql/blob/1cf01cd7d9aefca692f24ac067def4ea0d453133/packages/node/src/indexer/store.service.ts#L79

  • You will want to change it to ${this.schema}_custom_enum_${i} to make it unique.
  • Also, don't drop the type, we are not allow migration in db (any migration will require re-index).

@jiqiang90
Copy link
Contributor

@raycar5 We really appreciate your contribution, and we will adopt it. And we have some concerns here for the risks of the data, but we will continue to improve on the current implementation.

  • The interface for generated enum should use number for value instead of string , so this can be matching the implementation on the database layer.
  • Also the order of the member inside the enum is important , if the order changed we should consider it is not consistent.
    We expect codegen generate interface be like:
export enum TestEnum {
    Bar ,
    Foo ,
}

In another pr #532 we have tidy up the types in our packages, soon after merge yours, we will integrate it to our changes.

@raycar5
Copy link
Contributor Author

raycar5 commented Nov 5, 2021

Looking at the postgres docs indeed it seems enum order matters so the matching should be order sensitive.

However I do not understand what you mean by "should use number for value instead of string , so this can be matching the implementation on the database layer", the database layer uses strings to interact with enum fields.

If I understand your comment correctly, you're saying that you'll pick up where I left off right? So I shouldn't push the fix for the enum order checking.

@ianhe8x
Copy link
Collaborator

ianhe8x commented Nov 5, 2021

Yes, jiqiang90 will finish the rest along with the matching fix. The consideration of using number is for shortening the hashcode for enum fields. Depends on if pg accept number for enum field, we will change the enum to number or just do that in the toHashcode()

@ianhe8x ianhe8x merged commit aabdf4c into subquery:main Nov 8, 2021
@raycar5 raycar5 deleted the add_enums_descriptions branch November 9, 2021 13:20
@jiqiang90 jiqiang90 mentioned this pull request Nov 26, 2021
bz888 pushed a commit that referenced this pull request Jun 3, 2022
* Add support for enums and doc comments

* address PR comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants