Fixed #16429 -- Provided refresh option for FilePathField path choices.

[iamjameswalters]

This addresses Trac ticket #16429 requesting configurable caching of FilePathField options. Includes documentation and tests. All tests pass on my machine.

[nessita]

Hello @iamjameswalters, thank you for your first contribution!

I have a couple of comments, some are low-level and specific to the code, but before we get into that, I would like to discuss a higher level detail:

From my understanding of the proposed code, the file path choices get updated only when refresh_cache is called. I had this expectation that the file path choices would be automatically updated (as I think the ticket describes/requests). Would you please share your view of how this proposal solves the ticket? Is there something I'm missing? Could you, perhaps, share an example of how to test this?

The above is particularly relevant when comparing the current behavior in main with the proposed changes. My original test in main would show a list of files, and not update until I reload the page that holds the form. When using this PR, I can't get a refreshed file list until I refresh the page, which is the same as in main?

[nessita]

After I added the previous comment, I found this discussion in the developer mailing list, which made me realize that "automatically updating the list of file choices" could mean different things.

For the manual tests that I initially ran, I used the admin to show/use the form which does update the list of files on each page reload, presumably because it creates the form dynamically. So, I then created non-admin views that would use the forms more "traditionally" and I was able to see a behavior change between main (list of file choices are never updated, even on page reload) vs this branch where the list is updated based on the cache value.

Having said that, I'm not sure that having the cache new param being a boolean or a timedelta is a good idea. I would suggest we make this an positive integer (refresh_seconds?), with the following meaning:

• If >= 0, refresh after that many seconds (having 0 meaning "refresh now" or "never cache")
• If None, never refresh, i.e. cache indefinitely, which should be the default to be backwards compatible

Similarly, I wouldn't store the value as given, I would instead suggest that the timestamp at which the list should be invalidated is stored. Something like (untested):

if refresh_seconds is not None and refresh_seconds >= 0:
    refresh_at = now() + timedelta(seconds=refresh_seconds)
else:
    refresh_at = None

Then refresh checks would be:

if refresh_at and refresh_at >= now():
    # refresh choices

@iamjameswalters What do you think?

[iamjameswalters]

Hi @nessita ! Thanks for taking a look at this.

Yes, you've got it--sorry, I should have provided some more context.

The issue in the ticket as I understand it is some people used FilePathField expecting the choices to reflect whatever the current state of path was. However, FilePathField gets its choices whenever instantiated, which for the average person will happen whenever your forms.py is imported, which is probably when views.py first gets run. I've added a hook to Form that will call the refresh_cache() method on any Field that implements it whenever the Form is instantiated (which would happen every time a view is newly called).

As to your notes, I agree, having int or None is more consistent than a bool/datetime.timedelta mishmash. I've made that change.

As to storing the value vs. storing the timestamp after which to refresh, we would still need to store the original value somewhere in order to continue to calculate new refresh timestamps. But I think what you're after is to lighten the computational load when checking if a refresh is necessary, yes? We'd rather already have that datetime available for comparison than have to calculate on every Form load. I've modified this accordingly so that the refresh interval and the next refresh timestamp are stored. refresh_cache() uses the existing timestamp for comparison, and if refreshing, uses the stored interval to calculate the next refresh timestamp. Please let me know if I've understood you incorrectly, however.

[nessita]

Thanks for the ping @nessita. Thanks for the work on this @iamjameswalters.

Adding the _refresh_cached_fields() hook into the Form API, just for the purpose of solving this issue in FilePathField seems problematic to me. (I don't like the look of that at all 😳)

I agree and that's why I summoned more people :-)

I think the whole refresh_seconds move is likely too much API as well; in my experience there are really only two time-frames needed, that cover almost all use-cases: once on-load (which is the current behaviour), and this instance (which normally equated to per-request). I think if we handle those two, we can leave anything more complex to custom classes in userland code.

To solve the this instance (per-request) case, the usual approach is to override Form.__init__ and make the required adjustments there. (The usual example of this would be setting the queryset on a ModelChoiceField, the reflect the current user's needs.)

Moving the code from FilePathField.__init__() to a public set_choices(), would enable users to call that when instantiating their form if needed:

This is a great idea, and I very much like the fresh approach to the issue. I was reviewing this work with the ticket's description in mind, which says "configurable caching", but your proposal is more practical and I agree it solves the underlying issue. I do think we need to re-phrase the ticket though (I'll do that next).

As far as I can see, that would resolve the requirement from the ticket (for all but the most esoteric use-cases) without us needing to expand the form API, or ask users to explicitly decide on appropriate timeouts.

What do you think? 🤔

I agree with you: your proposed solution significantly reduces the scope of the changes and addresses common scenarios effectively. Pursuing the general solution would entail more complexity, new API design, and numerous changes, making the current simplification a more pragmatic choice for the majority of use cases.

Thank you for your valuable feedback! :brain: :heart_eyes:

@iamjameswalters What do you think?

[nessita]

Closing due to inactivity.