ic-labs / django-icekit

GLAMkit is a next-generation Python CMS by the Interaction Consortium, designed especially for the cultural sector.
http://glamkit.com
MIT License
47 stars 11 forks source link

Image.external_ref field has 255 characters limit #251

Closed markfinger closed 7 years ago

markfinger commented 7 years ago

We've got some edge-cases where the file paths are extremely long (255 chars exactly). As we're also storing the mtimes so we can diff the file-system, we're easily blowing these limits.

@jmurty while you're looking at #250, can you also increase the max_length up to 1000 or so?

jmurty commented 7 years ago

@markfinger @cogat I am very reluctant to increase the size of the Image.external_ref field in an ad-hoc way to account for what is arguably abuse of this field in this case.

Especially since we have 4 distinct places where a logically equivalent external_ref field is defined on models in GLAMkit, and changing the size in one place would then make the field inconsistent in a surprising way.

I propose that instead of an ad-hoc change, we take this chance to consolidate all the externally-referrable models to be consistent and DRY by:

As part of the consolidation, we could also take the chance to rename the external_ref field to something clearer like external_reference

And we could increase the field size which is probably reasonable anyway, and the same size would then apply to all logically identical fields through the system.

The main drawbacks with all these changes is it would mean:

markfinger commented 7 years ago

Small addition: I think we should still bump the char limit to something much, much larger to future proof ourselves.

jmurty commented 7 years ago

Another con with centralised external reference fields is that they would apply to Occurrence models in events, which should ideally be as lightweight as possible. Extra date time fields and a large external_ref works against that

cogat commented 7 years ago

There's no particular common way in which the external_ref fields are used. I think just bump up the field length oni Image.external_ref to something large or make it a TextField. The consolidation/generalisation can wait for another day.

jmurty commented 7 years ago

OK, I've just bumped the field size for images in particular. It's crappy, but it's done.