ForestAdmin / agent-nodejs

šŸŒ± Node.js agent for Forest Admin
GNU General Public License v3.0
67 stars 8 forks source link

Missing field rewrite OneToMany relation targets #1181

Open christian-mibex opened 2 months ago

christian-mibex commented 2 months ago

Expected behavior

We migrated a while ago from the express-sequalize agent to the nodejs agent. During that migration we wanted to keep the camelCase naming in the UI facing schema to avoid rework. As suggested here we rename the fields and collections.

We expect the same behavior for added relations:

    licenses.addOneToManyRelation('comments', 'licenseComments', {
        originKey: 'license_id',
        originKeyTarget: 'license_name',
    })

Actual behavior

The schema generation fails with the following stack trace

TypeError: Cannot read properties of undefined (reading 'columnType')
    at Function.buildToManyRelationSchema (/node_modules/@forestadmin/agent/src/utils/forest-schema/generator-fields.ts:134:26)
    at Function.buildRelationSchema (/node_modules/@forestadmin/agent/src/utils/forest-schema/generator-fields.ts:229:38)
    at Function.buildSchema (/node_modules/@forestadmin/agent/src/utils/forest-schema/generator-fields.ts:44:40)
    at /node_modules/@forestadmin/agent/src/utils/forest-schema/generator-collection.ts:51:42
    at Array.map (<anonymous>)
    at Function.buildFields (/node_modules/@forestadmin/agent/src/utils/forest-schema/generator-collection.ts:51:8)
    at Function.buildSchema /node_modules/@forestadmin/agent/src/utils/forest-schema/generator-collection.ts:18:20)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

When I debug the code, I see that the relation in the schema generation code has the snake_case name for originKeyTarget which then results in an undefinded field and brakes the schema generation with the stack trace above.

{
  type: 'OneToMany',
  foreignCollection: 'licenseComments',
  originKey: 'licenseId',
  originKeyTarget: 'license_name'
}

Context

Thenkei commented 2 months ago

Hello @christian-mibex,

Sorry for the late response. We gonna have a look at this next week.

As suggested here we rename the fields and collections.

āš ļø This only rename collections not field.

We will start by reproducting your issue and then have a closer look on how it can be resolved.

Kind regards, Morgan

christian-mibex commented 2 months ago

No worries, thanks for having a look!

What is then the proposed solution to migrate from express-sequelize (camelCase in code & forestadmin, but snake_case in database) to the nodejs agent without reworking the UI?

Best regards Christian

Thenkei commented 2 months ago

Hello @christian-mibex,

I've looked at your project Mibex_CRM (that's the one you're trying to migrate to the agent Node.js, am I right?).

For example, looking at our data you have a collection contactComments with a field followUpDate. So on our side nothing show an issue with camelCase and snake_case that could cause you loosing layouts once you do migrate.

If you have any more question about the migration I invite you to create a ticket on our community forum. We have a dedicated team that will be able to respond to you. šŸ™

For your initiale issue, we will try to reproduce it but we are having a team building right now so it could take a few days to give you a proper explanation/fix.

Kind regards, Morgan

Thenkei commented 1 month ago

Hello @christian-mibex,

We did release a small enhancement that return you clear message of the usable fields inside the customisation.

Let us know if it's clearer to you.

Kind regards, Morgan

christian-mibex commented 1 month ago

Hi

Let's exclude the migration discussion for now. I think the problem is different.

  1. I have a collection with a renamed field
  2. I want to use that renamed field in a addOneToManyRelation relation as target
  3. Because the typings.ts schema contains the not renamed fields, I am forced to use the un-renamed field in the declaration of relation, otherwise it won't compile.
  4. During the creation of the forestadmin schema, this code will fail with my error message in the initial post

In my opinion this is a clear bug.

By the way the renaming works fine for the originKey, a declaration like this:

    licenses.addOneToManyRelation('comments', 'licenseComments', {
        originKey: 'license_id',
        originKeyTarget: 'name',
    })

works perfectly, does the correct rewrites etc.

It's just that the target field does not get the same treatment.

I appreciate the improved error message! :clap:

I will also post in the community about the migration issues.

Thanks, Christian

Thenkei commented 1 month ago

Hey @christian-mibex,

  1. I have a collection with a renamed field

How do you rename the field ?

  1. Because the typings.ts schema contains the not renamed fields, I am forced to use the un-renamed field in the declaration of relation, otherwise it won't compile.

Ok, this is an issue. You should have the right typings properly generated.

It's just that the target field does not get the same treatment.

But why the target field should have an other name than name ? It's either the column name in the table or the name you use in the Sequelize declarations.

Could you share your configuration so that I can have a better understanding?

Just for the record, maybe the documentation is misleading.

I do have a model in my DB with the following declaration.

CREATE TABLE parties (
    id SERIAL PRIMARY KEY,
    post_id integer NOT NULL REFERENCES posts(id),
    body character varying(500) NOT NULL,
    date date,
    code_postal character varying(5),
    nombre_party integer
);

And in the agent I can use it properly with the snake case naming (it's totally normal). image


Ok, I think I go your issue. You want to create iso forest-admin-schema to avoid loosing frontend customization?

image

So you need to rename field in this case. https://docs.forestadmin.com/developer-guide-agents-nodejs/agent-customization/fields/import-rename-remove#renaming-and-removing-fields-and-relations

I will have a look at creating a plugin for you that allow to rename all collections fields from snake_case to camelCase..


I appreciate the improved error message! šŸ‘

We hope that it will help everyone. šŸ™

Kind regards, Morgan

Thenkei commented 1 month ago

Hello @christian-mibex,

Thank you for your feedback. šŸ™

You can use our new plugin to do the job for you.

We also improve the documentation by explaining correctly how to resolve those fields naming issues.

Kind regards, Morgan

christian-mibex commented 1 month ago

Hi @Thenkei

Sorry for the late reply, the email slipped through.

For completeness here are the answers to your questions:

do you rename the field?

We use the following code

    agent.addDataSource(dataSourceFactory,
        {
            exclude: ['db_migrations'],
            rename: toCamelCase // rename collections to camelCase
        }
    );

    agent.use(async ds => {
        for (const collection of ds.collections)
            for (const field of Object.keys(collection.schema.fields)) {
                collection.renameField(field, toCamelCase(field));
            }
    });

typing.ts

Indeed, the typings.ts should use the renamed fields. I did not look into the generation procedure but I guess it's built before the agent is customized.

naming examples:

I was not very clear, I wanted to point out that the schema generation does not work for the example in the initial post:

  licenses.addOneToManyRelation('comments', 'licenseComments', {
      originKey: 'license_id',
      originKeyTarget: 'license_name',
  })

but it does work if there is no snake_case name involved like here:

  licenses.addOneToManyRelation('comments', 'licenseComments', {
      originKey: 'license_id',
      originKeyTarget: 'name',
  })

your working example

Yes this works because you have not renamed the fields.

We could do this as well, but that means we would need to rewrite all the UI customization.

This mismatch between camelCase (in UI) and snake_case (in database) comes because we migrated from the sequelize-express-agent. The sequelize model was camelCase, but stored in the PostgreSQL DB in snake_case. Forestadmin only saw the camelCase schema. We built the UI based on that.

Now we migrated to the nodejs-agent. This means the sequelize layer is gone, and the agent created the schema/typings directly from the DB with snake_case. To counter this, we introduced the field renamings. This worked quite well, with the main downside being the typings.ts, and the required snake_case use in the backend.


Thanks for the plugin, I will try it out and will give feedback!

Thanks Christian

christian-mibex commented 1 month ago

Hi

The new rename plugin does only rename the fields, right? So it's the same as our code did:


    agent.use(async ds => {
        for (const collection of ds.collections)
            for (const field of Object.keys(collection.schema.fields)) {
                collection.renameField(field, toCamelCase(field));

The problem with snake_case in the originKeyTarget is still not solved.

Best regards Christian