feat: auto-increment for entity id #244

0xTitan · 2023-07-06T21:24:05Z

Hello.
Implementation done. Auto increment annotation can be added only once on entity definition. Only Int, BigInt and ID (for nested object) are managed.

Eg :
`scalar Text

type Post {
id: Int! @autoincrement
author: String!
content: Text!
tag: String
tx_hash: String!
created_at: Int!
created_at_block: Int!
poster: Poster
}

type Poster {
id: ID! @autoincrement
name: String!
}`

Unfortunately i'm struggling with knex implementation and the primaryKey definition. There is normally the possiblity to define if an autoIncrement field should be defined as primaryKey, this is not working when i'm setting it false. Due to that i can only managed one autoIncrement field per table. Maybe enought for now.

Eg :

Knex increment

knex.schema.createTable('users', function (table) { table.increments('id'); table.increments('other_id', { primaryKey: false }); });

I added two more tests :

should work with autoincrement
should work with autoincrement and nested objects

I put the constant needed under type.ts. Let me know if it's suitable.

The result under myssql for the schema above :

Sekhmet

@autoincrement can be handled as native GraphQL directive, like @derivedFrom

0xTitan · 2023-07-10T14:37:04Z

Ok thx, will check that.

0xTitan · 2023-07-26T21:06:49Z

Update done. New directive @autoincrement added.

Sekhmet · 2023-09-25T17:48:58Z

src/graphql/controller.ts

+   * @param schema
+   * @returns A map where key is table name and values a list of fields with annotation autoIncrement
+   */
+  private extractAutoIncrementFields(schema: string) {


This shouldn't be handled with regexes. Directive can be handled natively with GraphQL.

Hi, can you guide me to some example of what you want. From my understanding autoIncrement directive is not native in GraphQL so i need to implement the logic behind this new directive. Thank you.

As we extended GraphQL with our custom directive here, we can then access that directive directly from our GraphQL tree.

This way you can look at the field (id field in this example, and check if it has @autoincrement directive, same as we do with @derivedFrom:

checkpoint/src/graphql/resolvers.ts

Lines 182 to 186 in e6dd8b3

const directives = field.astNode?.directives ?? [];

const derivedFromDirective = directives.find(dir => dir.name.value === 'derivedFrom');

if (!derivedFromDirective) {

throw new Error(`field ${field.name} is missing derivedFrom directive`);

}

That means we don't have to bother with writing our own dummy parser for GraphQL to extract autoIncrementFields, and just check for field.astNode.directives here:

checkpoint/src/graphql/controller.ts

Lines 223 to 228 in 23698a4

if (

autoIncrementFieldsMap.size == 0 ||

autoIncrementFieldsMap.get(tableName)?.length === 0

) {

t.primary(['id']);

}

OK thx for the hint. I will check it and modify accordingly.

Sekhmet · 2023-10-19T18:03:55Z

@0xTitan thank you for your contribution so far. Are you still expecting to work on this issue? If not I could push it across finish line.

0xTitan · 2023-10-19T18:18:34Z

Hello @Sekhmet, sorry i didn't had time to continue on this, i was involved in other starknet project. I would like to finish this task. But if it's easier for you can push it across finish line and i can create a new PR later with correct implementation.
Thank you.

0xTitan · 2023-11-04T08:14:27Z

Hi @Sekhmet, i pushed changes to remove regex and use directive. Huge thanks for your help and your patience !

Sekhmet

Looks way cleaner thank you!

General thoughts:

can you revert any unrelated changes to the formatting? Most of it would be solved by running Prettier or yarn lint.
the comments added are a bit too much in my opinion as they explain pretty self-explanatory code, so they could be removed
we have added ORM since and it won't work well with it (as currently ORM requires you to create new instances of ORM models by passing ID to a constructor) - is this something you are also willing to take a look at? Otherwise we probably should at least prevent ORM models from being created for entities with autoincrement fields until it's implemented. If you don't want to work on that I could at this logic so we could get your PR merged.

Sekhmet · 2023-11-04T09:50:08Z

test/unit/graphql/controller.test.ts

@@ -56,7 +56,7 @@ type Vote {
    });

    it('should work', async () => {
-      const controller = new GqlEntityController(`
+      const schema = `


I'd use just a single test case to test everything (just put autoincrement and nested properties in here) or
use it.each to avoid duplication of code.

Sekhmet · 2023-11-04T09:54:35Z

src/graphql/controller.ts

+        //if there is no autoIncrement fields on current table, mark the id as primary
+        const tableHasAutoIncrement = this.getTypeFields(type).some(field => {
+          const directives = field.astNode?.directives ?? [];
+          const autoIncrementDirective = directives.find(dir => dir.name.value === 'autoIncrement');
+          return autoIncrementDirective;
+        })
+
+        if (!tableHasAutoIncrement)
+          t.primary(['id']);


This code duplicates quite a bit of logic from below.
I wonder if it wouldn't be cleaner to remove this initial lookup and instead use original forEach:

let tableHasAutoIncrement = false // ... this.getTypeFields(type).forEach(field => { // ... if (autoIncrementDirective) { tableHasAutoIncrement = true t.increments(field.name, { primaryKey: true }); } } // ... if (!tableHasAutoIncrement) { t.primary(['id']) }

Thanks a lot for the review. I updated code based on your comments.
Regarding ORM, i could take a look ?

Sekhmet · 2023-11-06T10:54:58Z

package.json

@@ -61,4 +61,4 @@
    "dist/**/*",
    "src/**/*"
  ]
-}
+}


Can you revert this change?

Sekhmet · 2023-11-06T10:55:11Z

src/graphql/controller.ts

+          const directives = field.astNode?.directives ?? [];
+          const autoIncrementDirective = directives.find(dir => dir.name.value === 'autoIncrement');
+
+          //Check if field is declared as autoincrement


Suggested change

//Check if field is declared as autoincrement

Sekhmet · 2023-11-06T10:56:10Z

test/unit/graphql/controller.test.ts

+      expect(createQuery).toMatchSnapshot();
+    });
+
+    it('should work with autoincrement and nested objects', async () => {


I feel like it could just be the only test case in this file (it should work).

0xTitan · 2023-11-06T21:50:50Z

Thanks for your comments. Update done.

0xTitan added 3 commits July 2, 2023 22:47

feat: auto-increment for entity id

2735914

feat: auto-increment for entity id

99b73f1

feat: auto-increment for entity id

05305c2

Sekhmet requested changes Jul 10, 2023

View reviewed changes

feat: auto-increment for entity id

23698a4

Sekhmet reviewed Sep 25, 2023

View reviewed changes

0xTitan and others added 2 commits November 4, 2023 08:17

Merge branch 'checkpoint-labs:master' into master

ab03db8

use directive to check autoincrement field

359e10a

Sekhmet reviewed Nov 4, 2023

View reviewed changes

fix format, comments, tests, duplicate loop

d49c317

Sekhmet reviewed Nov 6, 2023

View reviewed changes

Rm comments, one graph test, fix package.json

2e92f08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: auto-increment for entity id #244

feat: auto-increment for entity id #244

0xTitan commented Jul 6, 2023 •

edited

Loading

Sekhmet left a comment

0xTitan commented Jul 10, 2023

0xTitan commented Jul 26, 2023

Sekhmet Sep 25, 2023

0xTitan Sep 26, 2023

Sekhmet Sep 26, 2023

0xTitan Sep 27, 2023

Sekhmet commented Oct 19, 2023

0xTitan commented Oct 19, 2023

0xTitan commented Nov 4, 2023

Sekhmet left a comment

Sekhmet Nov 4, 2023

Sekhmet Nov 4, 2023

0xTitan Nov 4, 2023 •

edited

Loading

Sekhmet Nov 6, 2023

Sekhmet Nov 6, 2023

Sekhmet Nov 6, 2023

0xTitan commented Nov 6, 2023

	const directives = field.astNode?.directives ?? [];
	const derivedFromDirective = directives.find(dir => dir.name.value === 'derivedFrom');
	if (!derivedFromDirective) {
	throw new Error(`field ${field.name} is missing derivedFrom directive`);
	}

	if (
	autoIncrementFieldsMap.size == 0 \|\|
	autoIncrementFieldsMap.get(tableName)?.length === 0
	) {
	t.primary(['id']);
	}

feat: auto-increment for entity id #244

Are you sure you want to change the base?

feat: auto-increment for entity id #244

Conversation

0xTitan commented Jul 6, 2023 • edited Loading

Sekhmet left a comment

Choose a reason for hiding this comment

0xTitan commented Jul 10, 2023

0xTitan commented Jul 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sekhmet commented Oct 19, 2023

0xTitan commented Oct 19, 2023

0xTitan commented Nov 4, 2023

Sekhmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xTitan Nov 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xTitan commented Nov 6, 2023

0xTitan commented Jul 6, 2023 •

edited

Loading

0xTitan Nov 4, 2023 •

edited

Loading