defaultValue evaluation behavior should be the same in admin dashboard and local API

[ssyberg]

There's a rather lengthy discussion about this here: #2785

The upshot is there is a divergence in defaultValue evaluation between browser based operations and those identical operations via local API. I personally think this is actually a bug but I will defer to @jmikrut opinion that it is not (unless he is now convinced and wants to upgrade this to a bug 😉).

[ssyberg]

I just want to add here @jmikrut the more Payload gets used, the bigger a problem you're going to have with any divergence between dashboard and API create/update/delete operations. As the Payload ecosystem grows you're going to be supporting more and more programmatic interactions with Payload data. If people are to migrate to Payload from another CMS, this is going to be particularly tricky. Probably more importantly, if you guys are currently working on some form of database/data migration behavior, IMO this will also become a really hairy issue.

[jmikrut]

Hey @ssyberg — I would like to understand a bit more about exactly where there is a divergence between our dashboard and API operations. In #2785, I had meant to clarify that the following order is exactly the same in admin and backend:

1. conditions are run
2. default values are calculated
3. validations are run

This same baseline order happens in both places, and this is what I hope to get clarification from you on, because I'm not sure what divergence you are referring to.

One very important note here is that on the backend, hooks are also executed - but those are server-side only. As a result, they will never come into play within the admin panel. So that is a fundamental difference between the contexts.

On the backend, the order looks like this:

1. beforeValidate field hooks are run
2. conditions are run
3. default values are calculated
4. beforeChange field hooks are run
5. validations are run

Can you clarify what you mean by a divergence?

One other important note is that on the backend, validations and conditions are only executed one time - and on the admin panel, they are executed every time a field changes.

As you can see above, due to these different contexts, there are two unavoidable, contextual differences between admin and backend. But I'm not sure there is much to do about this and I think this is actually expected. What would you like to see changed?

[ssyberg]

Thanks @jmikrut I must have misunderstood a previous statement about defaultValue calculations... but I'm back to thinking this is just a bug then, I've been able to reproduce this in a simplified example with just a defaultValue and a beforeChange hook. The below example works 100% of the time I've tested it via admin dashboard and successfully populates the relatedId based on the defaultValue of the related field:

const Sandbox = {
    slug: 'sandbox',
    fields: [
        {
            name: 'related',
            type: 'relationship',
            relationTo: ['centers', 'posts'],
            hasMany: false,
            defaultValue: async () => {
                const response = await fetch(`http://localhost:3000/cms/api/centers?where[slug][equals]=global`);
                const { docs } = await response.json();
                const globalId = docs[0].id;

                return { value: globalId, relationTo: 'centers' };
            },
        },
        {
            name: 'relatedId',
            type: 'text',
            hooks: {
                beforeChange: [
                    ({ data }) => {
                        return data?.related?.value || '';
                    },
                ],
            },
        },
    ],
};

export default Sandbox;

Now if I run a create via local API, the relatedId fields remains blank.
payload.create({collection:'sandbox', data: {}})

The reason why I'm claiming this is a race condition is because if I add an async timeout to the beforeChange hook it reliably works via the create operation.

beforeChange: [
    async ({ data }) => {
        await new Promise((resolve) => setTimeout(resolve, 10));
    },
    ({ data }) => {
        return data?.related?.value || '';
    },
],

[jmikrut]

This is definitely not a bug, and is just a side-effect of your beforeChange action running alongside of your defaultValue calculation.

The reason that this works when you create a document from the admin UI is because your defaultValue function runs within the admin UI itself, so when you send your data through to the backend, that defaultValue is present already. So when your beforeChange hook fires, it will have the defaultValue already because it was already calculated and provided via the admin UI.

If you solely work with the local API, these two actions are running alongside of one another in separate async functions. The lifecycle event (the async function) for both your related and your relatedId fields are firing at the same time because they are siblings.

You are expecting that ANY beforeChange field hooks fire AFTER defaultValue calculations are done, but that is only true when you consider one field at a time.

When you consider one field at a time, these events DO happen in a sequential order, and the order is the same within admin UI:

• conditions are run
• default values are calculated
• beforeChange field hooks are run
• validations are run

But the thing that you are missing is that this "group" of functions executes at the same time for fields that are siblings. This means that your related field is performing these functions in parallel with them being performed on your relatedId field. So it certainly is a race condition, but it is a race condition that is caused by your code and would be expected by Payload.

Does this make sense? This is the performance optimization that I was mentioning in the prior thread. We minimize the amount of looping over field structures on the backend wherever we can to keep performance as high as possible.

It WOULD be possible to "abstract out" defaultValue calculations into a separate field traversal loop so that ALL fields' default values are calculated before ANY beforeChange field hook is fired, but that is a change and would be slower / require more looping to accomplish. This is what you are asking for, but it is not a divergence between admin and local API.

That said, I am definitely not opposed to abstracting out defaultValue calculation into a completely separate step that runs between beforeValidate and beforeChange. This is I think what you are looking for.

Alternatively, we could also move the following actions into the beforeValidate lifecycle event:

• conditions are run
• default values are calculated

That way we have no further looping required (it's done within existing loops) and you would be able to guarantee that defaultValues would be calculated all in advance of beforeChange hooks firing.

To me, that's the way to go here. What do you think?

[ssyberg]

Haha we've come full circle, and this has precisely been my point all along:

because your defaultValue function runs within the admin UI itself, so when you send your data through to the backend, that defaultValue is present already. So when your beforeChange hook fires, it will have the defaultValue already because it was already calculated and provided via the admin UI.

In my opinion this is the crux of the entire thing and absolutely represents a divergence in behavior between API and admin, I'm struggling to see how it's not.

So yeah to keep parity with admin I think I would suggest defaultValue be evaluated for every field before any beforeChange events happen. It's not immediately obvious to me if that should be within the beforeValidate lifecycle or not, but that seems reasonable at a glance and is in line with the documentation description of that hook: "Runs before the create and update operations. This hook allows you to add or format data before the incoming data is validated server-side." I would probably also expect defaultValue to be evaluated before any other beforeValidate hooks started.

guarantee that defaultValues would be calculated all in advance of beforeChange hooks firing

I don't want to come off too strong here, but this seems like something that you'd absolutely want to be guaranteed, the idea that the data in beforeChange might not represent the actual data feels very wrong to me.

Thanks again for hearing me out, sorry to keep pestering you about it but we're gearing up for a massive data migration and stuff like this makes me very nervous.

[jmikrut]

Hey @ssyberg — just a heads-up, @PatrikKozak and I are going to make some changes here shortly. I really appreciate the detailed conversation we've had here thus far and I think we have a way to solve this for good.

Keep an eye out. Should be within a week or so 👍

[ssyberg]

Amazing!

[ssyberg]

@jmikrut just wondering if this ever made it in or if there's a PR I can follow? Thanks!

[PatrikKozak]

Hey @ssyberg! We apologize for the late follow up on this - it's been a busy couple of months preparing for our 2.0 launch. But @jmikrut and I have gotten around to this and solved the issue with this PR here. We'll get this merged in shortly and this will be taken care of.

Thanks for your patience and for bringing this to our attention!

[ssyberg]

Great news, was just thinking about this this morning!