-
Notifications
You must be signed in to change notification settings - Fork 75
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
CSV task fails if uuid is used for active_storage_attachments.record_id #462
Comments
Unfortunately simply changing
|
Thanks for the report. Indeed we'd want to support |
The project only uses sqlite for testing. It seems that the first step is using both sqlite and postgres for testing. Is this the right direction @etiennebarrie ? |
Yes, if it's necessary. I wonder if we could just test UUIDs with sqlite? |
Mmm, maybe we could use blob as a primary key and store uuid. I'll take a look. |
I tried using string type as a primary key but it seems this comes with a lot of trouble because we cannot rely on database to generate ids. Personally testing both sqlite and postgress seems the way to go because it truly tests |
Changed primary keys to uuid, ran tests, and found that the current code depends on monotonically increasing ID more than I thought. For example, |
I'm not sure how which approach we should take:
For the first solution: from your investigation, do we need a migration, or can we rely on the existing (task_name, created_at) index? It seems that would be the case for last run and pagination (runs#show), but for the query you mentioned above (#462 (comment)), is that the case too? If we can avoid a migration, then the first solution is great: we don't need two code paths for UUIDs. But if we need more work for existing users, I think the second solution would be preferable. In these situations, I tend to do a spike so that I can see all the moving parts, even if I end up shipping change incrementally. I would favor having a branch where we can see everything we will need, even things aren't perfect or tests fail. |
Thanks for the advice.
Honestly, I'm not sure if we can avoid migration or not.
OK. I'm going to open a PR that simply changes pk type to uuid so we can discuss it more in detail. Then, I'm also going to open several PRs against the first PR branch. So that PRs stay small (though the last merge might end up large). |
I found myself in this situation and decided to hack it out with some monkeypatching and I thought I'd just offer this to anyone else who has a similar short-sighted need. I'm using Zeitwerk overrides as described here: https://guides.rubyonrails.org/classic_to_zeitwerk_howto.html#decorating-classes-and-modules-from-engines It uses ActiveRecordExtended for a window function and CTE because I was already using it, I'm sure we wouldn't want to pull this dependency into the gem but it could certainly be written without it.
The meat of the change above is substituting the old:
for the new scope:
With no other changes
And now I can run CSV tasks. No tests, no promises, works for me. |
Can we not refactor to use run timestamps in all of these cases? If someone is interested in issuing a PR where we use |
This issue has been marked as stale because it has not been commented on in two months. |
Any movement on this? We're still impacted. |
Are you interested in contributing? I think with the |
My team is currently pretty heads down, but we could potentially look at it later this year. If this issue gets closed as stale, can we reopen it later in the year when we're able to better contribute? |
This issue has been marked as stale because it has not been commented on in two months. |
Background
We use uuid for primary keys, thus
active_storage_attachments.record_id
is uuid. On the other hand,maintenance_tasks_runs.id
isbigint
. Therefore, when a CSV task tries to upload a CSV as anActiveStorage::Attachment
, the attachment fails to reference the CSV task's run record (MaintenanceTasks::Run
) because of the type inconsistency.Possible solutions
g.orm :active_record, primary_key_type: :uuid
when generating the migration file.Thanks in advance.
The text was updated successfully, but these errors were encountered: