17096 Do not omit "False" values from the request URL for cloned fields

[peteeckel]

Fixes: #17096

The current implementation leaves the URL parameters for boolean fields empty if their value is actually False, which leads to fields with False values not being cloned correctly.

[jeremystretch]

Have you tested this change using other models? A VRF, for instance, will not have its enforce_unique field cloned properly because the string "False" is interpreted as true.

[peteeckel]

I ran the NetBox test suite in the assumption that would secure the fix doesn't break anything. Obviously wrong ...

Given the fact that there is either the CF problem or the problem with cloning the enforce_unique field, the question is where exactly the problem is. As long as the string is constructed the was it currently is, CF cloning doesn't work properly, but presumably any non-null version is interpreted by the code that's responsible for cloning enforce_unique as True, which will break that code. That is definitely an inconsistency in the code base.

I can have a look at why the VRF code interprets "False" as True, but that might take some time.

[peteeckel]

@jeremystretch I had a look at the problem and there is a solution, but probably not a perfect one.

The interpretation of the value from the URL parameters is a Django utility function, and is applied to all boolean fields on initialisation. This results in the problem you mentioned. Custom boolean fields are not affected by that, because they aren't proper model fields after all, so it's safe to assume that all URL parameters starting with cf_ can (and need to) be passed the 'False' string if the value to be cloned is False. The new version of the fix reflects that.

However, I'm not sure what what will happen if someone choses to use fields of type NullBooleanField instead of BooleanField. NetBox currently doesn't, neither do I in my plugin, and so I don't have a proper testing environment. I think this could be handled as well by looking at the field type via _meta and doing special treatment if a NullBooleanField is detected (which is not the case for CFs, so the check for cf_ in the field name is still necessary), but for issue #17096 this patch is a sufficient solution.

[jeremystretch]

I'm afraid this isn't a tenable change: We cannot make arbitrary exceptions for certain types of fields. Instead, the form initialization logic may need to be updated to acknowledge explicit False (as opposed to undefined) values.

[peteeckel]

The problem with updating the form initialisation logic is that we can't do that indiscriminately either. There may well be a CharField that receives the string "False", in which case it isn't a very useful idea to modify it to the boolean value False.

So even when we modify the initialisation logic we still need to discriminate between field types that receive the data. Which is what I did now, this time on field type level.

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

[peteeckel]

@jeremystretch Did you notice that I made some changes to this PR - it's now pending closure and the bug is really annoying ...

[jeremystretch]

While the logic makes sense, I don't think NetBoxModelForm is the best place to address this. We want to ensure that this behavior is consistent for all forms across the application, and many forms don't inherit from this class.

Maybe normalize_querydict() would be a more appropriate place for this logic. We lose access to the individual form fields but it's probably reasonably safe to assume the string "False" implies the boolean value False.

[peteeckel]

it's probably reasonably safe to assume the string "False" implies the boolean value False.

While this may be true in most cases I would not implement code that relies on this assumption. It's just that, an assumption, after all, and errors resulting from applications actually relying on "False" being a string will be pretty hard to debug. That kind of reasoning is exactly where the referenced issue came from, and the solution of just assuming "False" will always refer to a boolean value will only hide the issue better.

The question is: Are there any forms not derived from NetBoxModelForm that are used in cloning objects?

[github-actions]

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.