LiteratureBuilder: avoid adding empty fields

[jmartinm]

In [3]: x = LiteratureBuilder(source='arxiv')

In [4]: x.add_language('')

In [5]: x
Out[5]: LiteratureBuilder(source="arxiv", record={'languages': ['']})

[michamos]

This is a conscious design decision. @david-caro wanted the buillder to be dumb, @jacquerie to be smart and do validation. I think it would make more sense for the builder to do validation of the subfields, i.e. the builder would contain

def __init__(self, source, record=None):
    # other init
    self.schema = load_schema('hep')

def add_language(...):
    # do whatever it is doing now
    validate(self.record['languages'], self.schema['properties']['languages'])

[jmartinm]

In principle I was thinking that filter_empty_parameters should take care of it (https://github.com/inspirehep/inspire-schemas/blob/master/inspire_schemas/builders.py#L229)

I created this issue because one of the workflows failed because we did (I don't know how):

builder.add_language('')

Amending filter_empty_parameters would have avoided this issue (adding the validation the form would have crashed for the user)

[michamos]

that function filters out empty kwargs. Here you provided an element of args:

In [1]: from inspire_schemas.builders import LiteratureBuilder

In [2]: x = LiteratureBuilder(source='arXiv')

In [3]: x.add_language(language='')

In [4]: x
Out[4]: LiteratureBuilder(source="arXiv", record={})

In [5]: x.add_language('')

In [6]: x
Out[6]: LiteratureBuilder(source="arXiv", record={'languages': ['']})

[michamos]

It probably is a bug in the builder though. @rikirenz, what do you think?

[jmartinm]

Yep, maybe filter_empty_parameters should deal with this situation of empty args as well.

[kaplun]

What is the conclusion of this issue?

[rikirenz]

1. This thing should not happen:

builder.add_language('')

Because by default we always pass a value:

anyway someone can change the post request and send an empty value.

2. The idea about the filter for empty values was this one:

   • When you have an optional parameter you are going to filter it
   • When you have a non optional parameter you are not going to filter it. Who is using the builder should care to filter mandatory parameters (because if he wants to add a empty field he should be able to do it)

3. Possible Solutions:

   • Add an if to check if the language is empty and in case we are not going to add the language field.
   • Put English as default value for the field language if empty.

Let me know what do you prefer 😃

[michamos]

Discussing IRL with @rikirenz and @jacquerie, we figured out it made more sense to filter everything for now. But maybe the whole approach of the builder should be rethought to be generated automatically from the schema (@jacquerie might explain more).

This problem was partially solved (only for empty fields in lists) by the _append_to function.

[kaplun]

Automatic generation from the schema will make the builder to change everytime the schema change, thus hindering the very purpose of the builder abstraction. What am I missing?

[rikirenz]

We were thinking about a way to create the actual builder directly from the schema. This implies to write a sort of compiler that is able to derive the builder from the schema. It is a cool idea but definitely is not a priority.

[kaplun]

You mean a way to kickstart a builder once, and then maintain it by hand, then, correct?

[rikirenz]

Well the compiler should be able to derive the builder every time that we do a change. It means that if the compiler is well written we should not touch the builder anymore.

[rikirenz]

Anyway before to fix this issue I open this #142. Because I am quite afraid to change the decorator filter without regression tests. For this reason from the moment that this issue it seems not so urgent, I would like to write the tests for the builder before and after fix this.

[rikirenz]

Depends #142

[kaplun]

Well the compiler should be able to derive the builder every time that we do a change. It means that if the compiler is well written we should not touch the builder anymore.

I am not understanding how the interface of the builder can remain stable if the builder is automatically generated from a changing schema. Anyway since this is not a priority, no need to convince me now 👍