500s when patching column to a different type

[pavish]

Description

When patching a column to a different type, having display_options or type_options in the request results in a 500 if the new type does not allow them.

Scenario 1:

• Create a text column
• Patch the column to URI type with null values for display and type options:

  {
   "type": "MATHESAR_TYPES.URI",
   "display_options": null,
   "type_options": null
  }
  

• Notice the following response with status code 500:

  [
    {
        "code": 4999,
        "message": "'NoneType' object is not iterable",
        "field": null,
        "detail": null
    }
  ]
  

Scenario 2:

• Create a text column
• Patch the column to URI type with an empty object: {} for display and type:

  {
   "type": "MATHESAR_TYPES.URI",
   "display_options": {},
   "type_options": {}
  }
  

• Notice the following response with status code 500:

  [
    {
        "code": 4999,
        "message": "Cannot find a matching serializer for the specified type uri",
        "field": null,
        "detail": null
    }
  ]
  

Expected behavior

• The response should not result in 500 for both scenarios.
• For Scenario 1, the request should succeed.
• For Scenario 2, the it would be easier for the client if the request succeeds. If that's against backend convention, the response should result in a 400.

[kgodey]

For Scenario 2, the it would be easier for the client if the request succeeds. If that's against backend convention, the response should result in a 400.

If display_options is set to an empty object, we should remove all display options and the request should succeed.

[Anish9901]

Can I work on this?

[silentninja]

@Anish9901 I have assigned the issue to you. This issue blocks a lot of frontend work, so a core contributor might take it if it takes too long to be completed(more than 2-3 days)

[Anish9901]

I'll try to get a PR in by a day or two @silentninja

[silentninja]

Thanks! @Anish9901

[silentninja]

If display_options is set to an empty object, we should remove all display options and the request should succeed.

@kgodey There are a few display options that are required and are created along with a type, for example, the currency symbol display option for the Money type. Without that display option the displayed data will look awkward and might end up with a bad UX, Should we allow users to remove all display options? Doing so would mean all the display options are optional

[kgodey]

@silentninja I think if the frontend sends a request to remove all display options, the backend should treat it as a request to reset the display options to their default state (i.e. the user is removing all customizations). We should not let the display options end up in a unusable state.

Alternatively, you can validate required display options, but the first option seems more flexible.

[silentninja]

• @kgodey @pavish What should be the default state for a JSON object like display options? Both None as well as empty object are valid values since we don't have any required display options. Right now we treat a None value as something set by the backend with a meaning that user is yet to set a display option(or something along the line of "display options is not enabled"). While an empty object is something set by the user voluntarily, which could be subjected to validation(maybe in future).

• Also, should every type be treated as if having an option to set display option or should we throw up an error if a type does not contain any display option and user tries to set a a display option object. Based on our discussions above, I am leaning towards considering every type can set a display option. I am looking at from an API perspective

[pavish]

@silentninja Here are my thoughts:

1. I think the default state should be None. Like you mentioned, an empty object is set by the client, for which currently we do not have any validation. But we might possibly have it in the future.
2. display_options and type_options are properties of the Column, so I think we should allow setting values for them for all columns, even though they depend on the column type and some types may not have them. We can throw errors for invalid entries, but we should allow null and {} for all.

[kgodey]

Sounds good to me.

[pavish]

@silentninja FYI, patch requests to the same type (Text -> Text) also seem to be failing with similar errors. I assume the fix for this issue would apply to that scenario as well.

[vrutik2809]

After a lot of debug on resolving this issue, i come to know that actual bug was on SimpleColumnSerailizer. This serializer is calling DisplayOptionsMappingSerializer which is extending ReadWritePolymorphicSerializerMappingMixin. ReadWritePolymorphicSerializerMappingMixin is trying to get email key from serializers_mapping dictionary defined in DisplayOptionsMappingSerializer which is currently not present there.
So temporary i have added (MathesarTypeIdentifier.EMAIL.value: EmailDisplayOptionSerializer), entry in serializers_mapping dictionary to check whether i am going in right direction or not and it is working.

Defination of EmailDisplayOptionSerializer:

class EmailDisplayOptionSerializer(MathesarErrorMessageMixin, OverrideRootPartialMixin, serializers.Serializer):
    format = serializers.CharField(max_length=255, required=False)

[vrutik2809]

And in addition to this, we need to define a display option serializer for each Mathesar data type which includes DisplayOptionsMappingSerializer so that in future this type of error could not occur. As currently, I am trying to change the data type of column from email to text then it is showing me the same error. Again the reason is the same: no entry for text type

[silentninja]

And in addition to this, we need to define a display option serializer for each Mathesar data type which includes DisplayOptionsMappingSerializer so that in future this type of error could not occur. As currently, I am trying to change the data type of column from email to text then it is showing me the same error. Again the reason is the same: no entry for text type

@vrutik2809 The source of error is correct, but instead of having a serializer for all the types, it would be better to have a default serializer that would ignore any display options for types that don't accept any display options. The reason is that we should be able to handle unknown types too that are reflected from the database too

[vrutik2809]

Can we do like if display_options is null then irrespective whether matching serializer is there or not just return else if display_options is not null and if matching serializer for that type is not found then only raise Exception? We can also add a prior check for display_options whether the respective type has a facility for that or not.

[Anish9901]

@pavish @silentninja Since this issue is now closed can we uncomment test_set_column_default_value?
https://github.com/centerofci/mathesar/blob/e33b5eed35882645f0344959fcc3f7e6c77662b6/mathesar/tests/integration/test_columns.py#L60-L79

[silentninja]

@Anish9901 Thanks for pointing it out. Can you send a PR to uncomment it out

[Anish9901]

Can you send a PR to uncomment it out

@silentninja Sure working on it.