`addField` gets called with empty records

Help me!
1

Feature(s) impacted

Adding smart fields, handling collection associations with Sequelize.

Observed behavior

Let’s imagine a simple user collection in which we add one-to-one relationships for a child to a parent, via a nullable parentId foreign key. A user may have 0 or 1 parent (depending on whether parentId is null). As we are running sequelize, we use hasOne and belongsTo to and from the user collection.

From there, let’s add a smart field:

agent.customizeCollection('user', collection => {
  collection.addField('displayName', {
    columnType: 'String',
    dependencies: ['firstName', 'lastName'],
    getValues: (records, context) => {
      console.log(records)
      // Note the toUpperCase
      return records.map(r => `${r.firstName} ${r.lastName.toUpperCase()}`),
    }
  });
});

This works perfectly on a user listing.
Now here’s the issue I encounter: when I go to view a specific user, getValues is always called 3 times. Twice for the parent and the child, and then once for the current user record.

If the user has neither child nor parent, then the records array will contain a single empty object twice. The third and last time, the records object will contain the current user.

That is, here is what appears in the console:

[ {} ]
[ {} ]
[ { firstName: 'John', lastName: 'Doe' }]

This causes a crash because .toUpperCase is accessed on undefined.

I’m thinking I may have bonked how I defined these relationships, so that Forest thinks users must have a parent and also must have a child. In which case the behavior could make sense…
This idea is supported by the fact that user.Child and user.Parent are both non-nullable in typings. I tried telling sequelize that parentId is nullable via allowNull, to no avail.

Expected behavior

Either I should build my models so that Forest understands this relationship is optional, or the fact that getValues can be called with empty objects is a bug. I’m trying to blame my code before moving on to blaming the framework =D

Context

  • Agent technology: nodejs
  • Agent (forest package) name & version: 1.40.1 with sequelize 6.37.3 and sequelize-typescript 2.1.6
  • Database type: MSSQL
  • Recent changes made on your end if any: Migrating from old agent.
2

Hello @kll :wave:

Let’s try to understand the behaviour you’re describing.

In this case it’s only call one, is it ? Because you’re displaying the field displayName within the list view, am I right?

So in the detail view you are probably displaying both parent and child?

And you probably have set the reference field ? In this case the relation to the record is not shown by the record id but by the field you selected.

Could you share your model definition or table definition? Are you using the @forestadmin/datasource-sql or @forestadmin/datasource-sequelize datasource ?

It doesn’t changed anything?

At this point, I don’t know for sure where the issue come from. It’s indeed weird that it tries to compute values for null records. For, sure adding and extra defensive programming in your code could help avoiding the issue but let wait a bit so that we can reproduce and have the final words on this. :pray:

Your code including a little monkey patch 
agent.customizeCollection('user', collection => {
  collection.addField('displayName', {
    columnType: 'String',
    dependencies: ['firstName', 'lastName'],
    getValues: (records, context) => {
      return records.map(r => r.firstName ? `${r.firstName} ${r.lastName.toUpperCase()}` : null),
    }
  });
});

Thanks for your feedback and thanks in advance for your answers.

Kind regards,
Morgan

1 Like
3

Hey there!

Indeed.

Yep, in the summary view. It is indeed also the reference field.

We’re using the Sequelize datasource and define the models with sequelize-typescript:

@Table({ tableName: 'Users', schema: process.env.DATABASE_SCHEMA })
export default class User extends Model {
  @PrimaryKey
  @Column
  declare Id: string

  @Column({ allowNull: false })
  declare FirstName: string

  @Column({ allowNull: false })
  declare LastName: string

  @HasOne(() => User, {
    foreignKey: {
      allowNull: true,
      name: 'ParentUserId',
    },
  })
  declare ChildUser: User

  @BelongsTo(() => User, {
    foreignKey: {
      allowNull: true,
      name: 'ParentUserId',
    },
  })
  declare ParentUser: User
}

(Ignore the capitalization)

Weirdly enough, no! What’s odd is that the FK is indeed nullable:
image

But the relationship isn’t:
image

I agree with you, I know there’s a simple solution to the symptom but I’d like to find the root cause.
Plus my abusive linter will scream at me:
image

4

Hey @kll,

Ok looks clear. But indeed if you don’t have any relation is shouldn’t try to compute anything.

I never used it but it just adds sugar syntax. Along with you might add the defaultValue: null.

I think we should see (understand) more things with some logs. Could you use the logger → loggerLevel: 'Debug'? It will show the SQL calls. I struggle to understand what is happening there (and I do not reproduce the same behavior).

To help us understand, maybe change the list of dependencies to see if there just an empty object or it contains an id. And log the whole object. :pray:

dependencies: ['id', 'firstName', 'lastName'],

We are in the same boat !

Kind regards,
Morgan

1 Like
5

Plop @morganperre,

Just tried, didn’t seem to change anything.

On a field which only needs a dependency on Id, this is what I get:
image
(Dashes added for readability)

Since you do not reproduce, I’ll build an example project from my code. I may not have time until next week though.

In the meantime I’ve checked the logged SQL and it seems that full objects are being fetched. I’ve pruned it down because there is a lot more in my migration project.

SELECT [User].[Id],
       [User].[CreatedAt],
       -- Snip: full User object, all fields
       [User].[ParentUserId],

       [ChildUser].[Id]                AS [ChildUser.Id],
       [ChildUser].[CreatedAt]         AS [ChildUser.CreatedAt],
       -- Snip: full User object for ChildUser, all fields
       [ChildUser].[ParentUserId]  AS [ChildUser.ParentUserId],

       [ParentUser].[Id]               AS [ParentUser.Id],
       [ParentUser].[CreatedAt]        AS [ParentUser.CreatedAt],
       -- Snip: full User object for ParentUser, all fields
       [ParentUser].[ParentUserId] AS [ParentUser.ParentUserId]
FROM [Users] AS [User]
         LEFT OUTER JOIN [Users] AS [ChildUser] ON [User].[Id] = [ChildUser].[ParentUserId]
         LEFT OUTER JOIN [Users] AS [ParentUser] ON [User].[ParentUserId] = [ParentUser].[Id]
WHERE ([User].[Id] = N'F7D56A51-7213-49EE-DDC0-08DC8F66E8F4');
6

Hey @kll,

After some back and forth with the team (and reproducing easily the behaviour you’ve described) I do have an answer.

This is an expected behavior. :sweat_smile:

If the relation you are depending on is null, you still need to return a value for that record (That’s not because you don’t have a relation that you don’t want to still compute that field).
Computed fields must be sent back with their original order of computation, so sending nothing would have caused issue to detect which computed belong to which record.

We could have sent null, but we didn’t just to prevent having multiple xx?.yy to check for dependencies existence before computing.

Typings may not be correct though.

So the good answer for the resolution is to handle the case. :confused:

agent.customizeCollection('user', collection => {
  collection.addField('displayName', {
    columnType: 'String',
    dependencies: ['firstName', 'lastName'],
    getValues: (records, context) => {
      return records.map(r => `${r.firstName} ${r.lastName?.toUpperCase()}`),
    }
  });
});

Let me know, if you understand this global behavior. I do understand that this is weird in the one to one relations with a reference field.

Kind regards,
Morgan

1 Like
7

This is hilarious :smile:
Okay so defensive programming and linter screaming it is!

I would definitely consider this a typings issue. They should be fixed to include an empty object as a potential value in records. Maybe the use of a Symbol here could be nice?

Here is the lint issue caused by your offered fix:
Lint issue screenshot

I worked around it by specifying the type explicitly and wrapping it in a Partial:
Code screenshot
Which is a pain tbh :frowning: This is very easy to forget and it will completely crash the page. I can see myself writing new code in a couple of weeks and expletively stepping on this particular lego.

Is this something the team has any intention to fix in a shortish term?

… I will also open a separate thread because I would really love to see some other typings :slight_smile:

In the meantime, thank you again for the time poured into this.

2 Likes
8

Indeed, strict typings configuration are not well supported and need workaround. :confused: Initially, the idea was to make the interfaces the simplest possible but it comes with some disagreement/troubles.

I do know that we worked on strict null checking within the typings but we may need some other improvements.

You’re welcome !

Kind regards,
Morgan

1 Like
9

I really don’t know, we always try to improve the global experience. We could indeed try to improve typings but it’s not always simple to generate them dynamically (when using dependencies fields for example). So yeah Partial might be better.

You can open a PR to help us contribute and we can help you supporting more use cases if you want to speedup the process. :pray:

Regards,
Morgan

1 Like
10

Hi @morganperre,

Thank you again for your replies and your time.

I’ll check the repos to see where this is generated and whether I can help in any way, should I have the time to do so :slight_smile:

1 Like