Default behavior is trying to PATCH/update primary key?

[braco]

I'm trying to update a single record, and seems like ra-d-pg is performing a PATCH to the endpoint, with the ID / primary key in the payload:

PATCH /rest/v1/table?id=eq.1 { id: 1, name: "foo" }

Because the id column was created with something like:

id INT GENERATED BY DEFAULT AS IDENTITY

and the above seems to be trying to update the id column, I'm getting this error:

column "id" can only be updated to DEFAULT

Is this expected behavior?

[christiaanwesterbeek]

This dataProvider currently PATCHes this body.

https://github.com/raphiniert-com/ra-data-postgrest/blob/c0cbecbd853593dac2e9835e74a34ea94424908e/src/dataProvider/index.ts#L309-L312

So it also adds the id, which in the case of a PK column with a generated default is not wanted.

We could think of a way to make it configurable which columns should be ignored (in the body of a create/update call) for a particular resource. Maybe we could use the 4rd argument to create the dataProvider (primaryKeys); that we not only pass what the primaryKey is per resource, but also that we can pass which fields to ignore. Or we use the meta option again, but that you would have to repeat everywhere for that resource...

[braco]

Do you know what ra-supabase is getting out of their resources config object? Something like that seems like a natural home, but I'm unsure of why its necessary otherwise: https://github.com/marmelab/ra-supabase#full-text-search-support

Since react-admin requires an id field by default, would this cover default cases?:

// only include id if it has value and is different from existing value
if (body.id === searchParams.id || body.id === "") { omit from body }

The second check is there because creating records didn't work either due to {id: ""}

Might be wise to do all of this through configurable functions so it can be composed with custom logic(?): excludeInUpdate((details) => ())

Otherwise a 4th argument to dataProvider seems like a good idea to me, so it's only defined once. edit: nevermind on all the other stuff above, just saw the section in your docs about compound keys, that makes sense there:

const dataProvider = postgrestRestProvider(API_URL, fetchUtils.fetchJson, 'eq', new Map([
  ['some_table',    ['custom_id']],
  ['another_table', ['first_column', 'second_column']],
]));

[christiaanwesterbeek]

To summarize; it maybe a good idea to do like ra-supabase. Where our dataProvider only configures the primaryKeys per resource, we could also configure what fields to ignore. You would also want ignored fields for computed columns for example (you can't create/update those either!).

This would mean implementing the following:

• upgrade our current 4th argument from primaryKeys: Map<string, PrimaryKey> to something that per resource can hold a configuration object with the PrimaryKey, ignoredFields(, and more config that we might think of in the future)
• respect ignoredFields in the create and update methods

[braco]

That sounds good! You could also be more specific with something like doNotUpdatePrimaryKeys: true if you need to pack more discrete PK logic in there, or less specific with doNotUpdateFields: [] to more intuitively cover cases like computed columns. ignoredFields sounds like it might apply to selects, etc.

edit just a thought without thinking side effects through: update/inserts could be written/have the option to be written as INSERT ON CONFLICT (pk) DO UPDATE

[scheiblr]

thank you @braco and @christiaanwesterbeek, I want to tackle this issue, soon. Therefore, could we summarize, what the conclusion of the entire discussion here is? Although I have some idea, I'm a bit confused, after reading it and would like to be sure what to do in order to end up with a clean solution.

[christiaanwesterbeek]

I think there are 2 solutions provided here to the original problem.

1. doNotUpdatePrimaryKeys: true
2. doNotUpdateFields: []

(using the names that @braco came up with, because they seem fine and understandable)

And it is suggested that we provide that configuration when creating the postgrestRestProvider (as opposed to sending the configs as mutationOption/meta in each request again and again).

I have a slight preference fordoNotUpdateFields, because they can be used to cover cases like computed columns. However than you want to apply that to CREATE queries, as computed columns in a database cannot be inserted or updated. I asked GPT4 to continue this thought process. Here's its answer.

Yes, since the configuration option applies to both CREATE and UPDATE queries, it would be better to give it a more inclusive name. Here are some naming ideas:

• ignoreFields: A simple and general name that implies these fields should be ignored during both CREATE and UPDATE operations.
• nonEditableFields: Indicates that these fields cannot be edited during both insertion and update processes.
• readOnlyFields: Suggests that these fields are read-only and should not be changed during CREATE or UPDATE operations.
• skipFields: Implies that these fields should be skipped during the processing of both CREATE and UPDATE queries.

I recommend using ignoreFields or readOnlyFields, as they are more descriptive and clearly communicate the intended behavior.

The last thing that I want to suggest is to may be not add another specific parameter to creating the dataProvider. But this is just a feeling. I would suggest to add some kind of configuration object with the ignoreFields/readOnlyFields/doNotUpdateFields (whichever you choose), so that we can add more config in the future without creating crazy amounts of parameters. Again just a feeling.

[scheiblr]

Thanks a lot for your effort. Especially, I totally agree that we should switch from multiple parameters to a config option. As we are in alpha, I think, it's the right time for such a breaking change. Thanks for bringing that one up. About the rest I still have to think and how we could cleverly do that.

[scheiblr]

ok, we switched over to config interface instead of parameters. Thus, the foundation is set to tackle this issue 👍

[scheiblr]

Hey @braco, we just released v2.0.0-alpha.4. The main issue you mentioned in the beginning concerning the behavior of the id's should be solved by now (without manual configuration) and I think therefore the core of the present issue. If I miss something, please feel free to reopen the ticket.