callback fails to save empty files

dmsimard commented 4 years ago

What component is this about ?

callback plugin

What is happening ?

Noticed when running openshift-ansible:

2020-01-03 20:30:19,014 WARNING django.request: Bad Request: /api/v1/files
2020-01-03 20:30:19,055 ERROR ara.clients.http: Failed to post on /api/v1/files: {'playbook': 1, 'path': '/home/fedora/src/github.com/openshift/openshift-ansible/roles/nuage_ca/vars/main.yaml', 'content': ''}
2020-01-03 20:30:34,684 WARNING django.request: Bad Request: /api/v1/files
2020-01-03 20:30:34,727 ERROR ara.clients.http: Failed to post on /api/v1/files: {'playbook': 1, 'path': '/home/fedora/src/github.com/openshift/openshift-ansible/roles/openshift_management/handlers/main.yml', 'content': ''}

What should be happening ?

It should be possible to post empty files.

LaurentDumont commented 4 years ago

I hit that randomly when including a file that was empty (or rather in the process of getting written). I was thinking on how to solve it and had two different ways. I don't think it's going to come up super often, but it should definitely be handled in some way.

We could detect that the file is empty during the Callback action and fill it in with some placeholder text. Similar to what is done when content is ignored. That said, I don't really like it since it's somewhat fake.
Taking a quick look at the code of the API server, it looks like ARA encode and decodes the content of the file into the database. At first, I just added the SQL migration to remove the non NULL constraint. That did not seem to be enough and I assume that the API needs to be aware that the content can be empty when the data is passed through the serializer.

Let me know if something is more in-line with your vision and I'll see if I can cobble something up.

dmsimard commented 4 years ago

Thanks for looking at this @LaurentDumont, I just now had the time to dig into this a bit.

Taking a quick look at the code of the API server, it looks like ARA encode and decodes the content of the file into the database. At first, I just added the SQL migration to remove the non NULL constraint. That did not seem to be enough and I assume that the API needs to be aware that the content can be empty when the data is passed through the serializer.

Yes, I actually tried that with the allow_blank parameter in the serializer here: https://github.com/ansible-community/ara/blob/821b0fe8801fbcfccce5202d1360aca921aecde8/ara/api/serializers.py#L439

It fails quite spectacularly: from what I can tell, it looks like django-rest-framework doesn't end up running the get_or_create for the FileContent field (because it's blank?): https://github.com/encode/django-rest-framework/blob/8cba4f87ca8e785d1a8c022a7a8ea9649e049c11/rest_framework/fields.py#L824-L832

This, in turn, makes it so when the File serializer attempts to save the file, it has an empty string for content rather than a FileContent object and it ends there.

We could detect that the file is empty during the Callback action and fill it in with some placeholder text. Similar to what is done when content is ignored. That said, I don't really like it since it's somewhat fake.

Yeah, it is not ideal but I'd suggest we go with this option for the sake of simplicity for now. We can always revisit it later if we feel it's not appropriate. What do you think ?

dmsimard commented 2 years ago

A user reminded me of this issue and we should implement the placeholder text as a workaround.

I'll take care of prioritizing it in time before the next release if no one has gotten to it first.

ansible-community / ara