3YOURMIND / django-deprecate-fields

This package allows deprecating model fields and allows removing them in a backwards compatible manner.
Apache License 2.0
173 stars 19 forks source link

Raise exception when deprecated field is accessed #8

Closed POD666 closed 5 years ago

POD666 commented 5 years ago

Not sure if it matches your vision but it makes sense for me.

Here is how it looks like:

In [123]: MyModel().myfield
---------------------------------------------------------------------------
AccessDeprecatedField                     Traceback (most recent call last)
<ipython-input-72-d46d25d6eb90> in <module>()
----> 1 MyModel().myfield

<ipython-input-70-3dcc944f58f2> in raise_exception(self)
     16 def raise_exception(self):
     17     """replacement for deprecated field"""
---> 18     raise AccessDeprecatedField("Deprecated field shouldn't be used anymore!")
     19 
     20 

AccessDeprecatedField: Deprecated field shouldn't be used anymore!

In [123]: MyModel().myfield = 4
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-73-ad458d8e91f7> in <module>()
----> 1 MyModel().myfield = 4

AttributeError: can't set attribute

You might want to change forbid_access to False to keep old behavior by default. Also, there might be better texts/names to be used.

dxvxd commented 5 years ago

Nice idea, but not suitable in our case. Basically, we don't want exceptions to be raised, that's why return_instead parameter is there.

vanschelven commented 3 years ago

@dxvxd could you expand on why it is not suitable in your case or even in general?

My understanding of the (or simply a?) use case for this package is in the context of backwards compatible migrations. i.e. it's useful in making a field disappear from your codebase, while maintaining it in your database such that older versions of your codebase don't crash because it is missing. In fact this is the use case that's being described in the referenced blog post

In that use case, hard failure would seem perfectly acceptable to me: it most closely matches what would happen if you would have proceeded "normally" (i.e. without trying to stay backwards compatible), since in that case you would simply remove the field resulting in an AttributeError. Hard failure is also more likely to ensure that your stack trace reflects the line in which an error was made, rather than somewhere down the line.

dxvxd commented 3 years ago

Use case is described in blog post you mentioned: you have working deployment and you need to upgrade it to latest version. During upgrade process, while migrations are running, you don't want to raise erros and break request-response cycles of current users.

vanschelven commented 3 years ago

I'm not quite sure I understand what you're saying here, so let me be even more explicit in my understanding of the typical workflow to see where te misunderstanding is. As I see it, the relevant pieces here are 2 versions of the code ("old" and "new"). 2 of these may be in play at the same time on different servers, connecting to a single database.

The code proposed here would, AFAICS, not raise errors in either of these:

There is of course a third thing that comes into play, namely the running of the migrations themselves. But there no actual models are used, so this code does not come into play AFAICS.