Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fixed behavior for updated_at field when updating the model #166

Merged
merged 4 commits into from
May 4, 2024

Conversation

SergeyKomVl
Copy link
Contributor

@SergeyKomVl SergeyKomVl commented Apr 30, 2024

Description

  • fixed behavior for updated_at field when updating the model

    When the model is created, it is not necessary to fill in the 'updated_at` field.
    But when updating the model, it needs to be updated automatically.

  • hotfix

    Returned the default value for the field.

Closes

@cofin
Copy link
Member

cofin commented May 2, 2024

In this case, the updated_at is generally populated by the model listener added here. While this may not be needed, it was originally done because it supports polymorphic relationships. I would be open to changing this if there's something that maintains that functionality.

Re: the initial population, I don't disagree that it should not be populated initially, but I often found that it was easier to work with an object you know is not null.

Let's see how the tests suite executes and we can see if anything different is required.

@SergeyKomVl
Copy link
Contributor Author

@cofin
I fixed it, thanks.

I didn't notice that this field is required in the database.

As for your explanation, I agree: it should be more convenient with a non-null value.

@cofin
Copy link
Member

cofin commented May 3, 2024

@SergeyKomVl quick question, are you using advanced alchemy in Litestar or on it's own? The reason I ask is that this should be taken care of by the listener that gets attached. So, in theory, you don't even need this configuration added to the model.

However, it occurred to me that, depending on how you are using the library, you may not invoke a configuration object.

Take a look at the test cases here and let me know if you still need this change.

@SergeyKomVl SergeyKomVl requested review from a team as code owners May 3, 2024 20:54
Copy link

github-actions bot commented May 3, 2024

Documentation preview will be available shortly at https://litestar-org.github.io/advanced-alchemy-docs-preview/166

@cofin cofin merged commit e049324 into litestar-org:main May 4, 2024
11 checks passed
@SergeyKomVl
Copy link
Contributor Author

@SergeyKomVl quick question, are you using advanced alchemy in Litestar or on it's own? The reason I ask is that this should be taken care of by the listener that gets attached. So, in theory, you don't even need this configuration added to the model.

However, it occurred to me that, depending on how you are using the library, you may not invoke a configuration object.

Take a look at the test cases here and let me know if you still need this change.

@cofin
I just started learning litestar.
Thanks for the links to the cases!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants