Required "Id" field is not auto generating

bamzi commented 6 days ago

Hi,

Updating my app from version 0.22.21 to 0.23.2

After applying the changes to make the app function again, on new record model Save, the 'Id' is no longer auto generating and data is being saved to DB with PK, id, + created & updated fields being empty.

Sample of my model and save method

type Project struct {
    core.BaseModel
    // Fields
    Title    string                  `db:"title" json:"title"`
    Tags     types.JSONArray[string] `db:"tags" json:"tags"`
    Status   string                  `db:"status" json:"status"`
    // Relations
    UserId    string       `db:"user" json:"user"`
    User      *core.Record `db:"-" `
}

func (l *Project) Save(app core.App) error {
    if err := app.Save(l); err != nil {
        return fmt.Errorf("save list (%s): %w", l.Id, err)
    }
    return nil
}

Expected behaviour

When instantiating a new struct project := new(Project), adding data to fields and establishing the relations. On save, the record should be added to project collection with id auto generated as with created (if now new) and updated before insert.

Second Problem

collections with id as PK (required non-empty) but en empty id field in the record is still inserted into DB. The expected result should be a DB error to indicate required field is missing and reject insert the record item.

[edit]

Third issue

Just realized created and updated are not set either and all moved out of BaseModel. Are we expected to manage them manually?

Side note, I can't find any articles or philosophy for change from 0.22.x to 0.23.x. I thought the removal of Echo and merge of dao and other modules into core as a simplification and dependency reduction. But it feels more like Pocketbase 2.0. Nothing wrong with that, i'm curious as for me the main selling point of Pocketbase is embedding into own app.

FYI, my app is 35k+ lines of code, 20+ collections that heavily depends on explicitness of struct models that reflect the schema structure of db tables.

my tech stack in case interested

Monorepo, pure GO full stack
2x Pocketbase apps
Templ
HTMX
Tailwind
server side rendering only (no more json, js or client side api requests)

ganigeorgiev commented 6 days ago

Side note, I can't find any articles or philosophy for change from 0.22.x to 0.23.x. I thought the removal of Echo and merge of dao and other modules into core as a simplification and dependency reduction. But it feels more like Pocketbase 2.0. Nothing wrong with that, i'm curious as for me the main selling point of Pocketbase is embedding into own app.

FYI, my app is 35k+ lines of code, 20+ collections that heavily depends on explicitness of struct models that reflect the schema structure of db tables.

Honestly I don't understand from where this entitlement come from and why people behave like somehow I own them something. There is no v1 in the first place to call it v2. I'm NOT selling anything nor I'm here to try to convince you to use PocketBase. If you find the project useful great, if not - leave a feedback or move on. But I'm tired reading people complains and demands especially when I think I've been more than clear that backward compatibility is not guaranteed and that this is just a project that I've created for my own use case (there is even a pinned section in the new FAQ).

As highlighted in the release notes, you don't have to migrate if you are satisfied with v0.22 features set or don't have the time to upgrade.

The plans for refactoring was first announced in https://github.com/pocketbase/pocketbase/discussions/4355. Many of the changes are actual cosmetic on the surface (not everything but most of these changes are listed in https://pocketbase.io/v023upgrade/go/).

The problem in your case seems to be that you are using custom record models for a lot of things, including persistence, but custom record models were mostly intended for scanning/querying collection data (often aggregations with other collections). But they have major limitation since they don't trigger the related record hooks when persisted, nor execute the related collection validations, which often resulted in strange data integrity issues if users are not careful enough. This is why the custom record models were superseded by RecordProxy.

BaseModel no longer have autogenerated id because with v0.23 we allow users to customize the system collection fields (including the id length and the autogenerate pattern). The created/updated system fields also are now converted to autodate optional regular fields and they don't have any special meaning outside of the Record model (you'll find in the dashboard that they are removable). Or in other words, if you want to continue using core.BaseModel then you'll have to set those fields on your own (for the id you can use core.GenerateDefaultRandomId() but note again that when persisting custom structs the record validators are not checked and it is essentially a direct db write).

My recommendation is:

if you don't have the time to upgrade, stay at v0.22
if you do want to upgrade, consider operating with only the core.Record or alternatively convert your custom record models to RecordProxy

For other questions, please use the Q&A discussions as the threaded format is more suitable for this type of posts.

bamzi commented 6 days ago

No, not entitlement --> PANIC

But they have major limitation since they don't trigger the related record hooks when persisted, nor execute the related collection validations, which often resulted in strange data integrity issues if users are not careful enough. This is why the custom record models were superseded by RecordProxy....

So flexibility and control got sacrificed because of dumb mistakes! When i started a few months ago, I read every line of your code before deciding to use it. The developer experience was great. The new direction is no longer for me.

I am not going to pretend that I understand you or agree with the new direction. It doesn't matter. Criticism is a good thing if you learn how to harness it. The moment we expose anything to public we are subject of opinions and judgement but we do it anyways. You're a good engineer ... best of luck. Maybe our paths will cross one day and we'll have a coffee chat.

ganigeorgiev commented 6 days ago

So flexibility and control got sacrificed because of dumb mistakes!

No, it is exactly the opposite - because of the new flexibility (aka. being able to change the options of the system fields) having custom record models doesn't really work because you can accidentally save an invalid state. The new generalized app.Save() (that runs the model validations by default) and the RecordProxy (for when you want typed access) is intended to solve these issues.

But again, I'm not here to convince you to use it and I will not argue further.

bamzi commented 6 days ago

I apologize. I rushed out of panic.

pocketbase / pocketbase