subsquid / squid-sdk

The main repo of the squid SDK
Apache License 2.0
1.23k stars 151 forks source link

Allow _ prefix for type and prop #174

Closed jasl closed 1 month ago

jasl commented 1 year ago

Allow _ prefix seems secure.

My case is, on-chain, an NFT item full represent is (CollectionId, ItemId), in squid, the below code will have a conflict

type NftExtendInfo @entity {
  id: ID!

  nftItem: NftItem! # It generates a varchar column nft_item_id to the database to store the foreign key, the format would like `${NftCollectionId}-${NftItemId}`
  nftItemId: Int! # This is the pure on-chain NFT item id
}

By introducing _ prop prefix, I would like to change it to

type NftExtendInfo @entity {
  _id: ID!

  _nftItem: NftItem!
  nftItemId: Int!
}

In addition, I also introducing _ prefix for type, I'm not use that, but I think it helpful for some cases

jasl commented 1 year ago

BTW, when I try run make test in local, npm i failed

npm ERR! code E404
npm ERR! 404 Not Found - GET https://registry.npmjs.org/gql-test-client - Not found
npm ERR! 404
npm ERR! 404  'gql-test-client@^0.0.0' is not in this registry.
npm ERR! 404
npm ERR! 404 Note that you can also install from a
npm ERR! 404 tarball, folder, http url, or git URL.
belopash commented 1 year ago

_property and _Property will collide. I think correct regex is ^_?[a-z][a-zA-Z0-9]*$

jasl commented 1 year ago

_property and _Property will collide. I think correct regex will be ^_?[a-z][a-zA-Z0-9]*$

Thank you, yours is better, updated

jasl commented 1 year ago

hello?

jasl commented 1 month ago

ping

eldargab commented 1 month ago

Hey, sorry for not responding!

Actually _ prefix were reserved for technical fields and that was used in a couple of places. We can eliminate this limitation, but that will require a bit more than merging PR and right now we don't have time for that.

jasl commented 1 month ago

Hey, sorry for not responding!

Actually _ prefix were reserved for technical fields and that was used in a couple of places. We can eliminate this limitation, but that will require a bit more than merging PR and right now we don't have time for that.

Yeah, I hope you can put it in your backlog.

Although my example is invalid now, but our Solana and EVM contracts have fields such as id, type, or tokenID, I want columns in Squid to keep the same names as our contracts to reduce misleading, and Squid's ID fields is less useful in dAPP in my experience

jasl commented 1 month ago

The default typeORM's convention of association columns is difficult to debug. I remember this PR because I encountered a naming collision, which took me an hour to figure out.

belopash commented 1 month ago

Alternatively, you can use another graphql server, for example https://www.graphile.org/postgraphile/

Usage example: https://github.com/subsquid-labs/squid-postgraphile-example/

In this case you don't need all openreader releted stuff, like codegen and schema file