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 id_partition interpolation with BSON::ObjectId #82

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jarthod
Copy link

@jarthod jarthod commented Feb 27, 2022

Hey all 👋

I noticed recently that id_partition interpolation was not working any more in my project, it was always nil.
As I investigated I noticed the reason is quite simple, the code expects the model instance id to be a String, whereas it's a BSON::ObjectId (and it has been so for a while in mongoid).

I suspect it was probably a String in earlier version of mongoid? At some point it was working fine in my code because I have old folders with the id_partition but I can't tell exactly when it stopped working because I delete old files regularly.

Anyway here is the fix I suggest (with an addition test for that): I added BSON::ObjectId to the supported types and .to_s to convert it to a String before the scan. (.to_s is a no-op if already a String). I tested this fix in my project and it restored the working id_partition interpolation.

New test before my fix showing the problem:
image

Let me know if you have any questions!

@jarthod
Copy link
Author

jarthod commented Feb 27, 2022

BTW one thing I noticed here also is that this id_partition is pretty long and costs a lot of folders (inodes), unless you have a number of records that no machine can even store, you'll create a lot of unique folders containing only one record. paperclip default (for string ids) is to take the first 3 groups of 3 letters: https://github.com/kreeti/kt-paperclip/blob/master/lib/paperclip/interpolations.rb#L186 which means only 3 levels of folders and is a more sensible default IMO for the average use-case.

WDYT about changing this? as the behavior was broken and we're fixing it maybe it's a good time to change the default at the same time so people only have to deal with this change once?

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.

1 participant