search

OpenEnergyPlatform / omi

Repository for the Open Metadata Integration (OMI). For metadata definition see metadata repo:
https://github.com/OpenEnergyPlatform/metadata
GNU Affero General Public License v3.0
7 stars 4 forks source link

Omi fails to parse valid(?) string with foreign keys as null #34

Closed christian-rli closed 4 years ago

christian-rli commented 4 years ago

Omi is unable to parse this metadata string. As far as I can tell, the string contains a valid empty foreign key, but omi seems not to think so. Any idea as to why that is @MGlauer ?

omi translate -f oep-v1.4 cams_timeseries.json 
Traceback (most recent call last):
  File "/home/christianh/anaconda3/bin/omi", line 11, in <module>
    sys.exit(main())
  File "/home/christianh/anaconda3/lib/python3.7/site-packages/omi/cli.py", line 49, in main
    cli()
  File "/home/christianh/anaconda3/lib/python3.7/site-packages/click/core.py", line 764, in __call__
    return self.main(*args, **kwargs)
  File "/home/christianh/anaconda3/lib/python3.7/site-packages/click/core.py", line 717, in main
    rv = self.invoke(ctx)
  File "/home/christianh/anaconda3/lib/python3.7/site-packages/click/core.py", line 1137, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/christianh/anaconda3/lib/python3.7/site-packages/click/core.py", line 956, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/christianh/anaconda3/lib/python3.7/site-packages/click/core.py", line 555, in invoke
    return callback(*args, **kwargs)
  File "/home/christianh/anaconda3/lib/python3.7/site-packages/omi/cli.py", line 35, in translate
    obj = from_dialect.parse(infile.read())
  File "/home/christianh/anaconda3/lib/python3.7/site-packages/omi/dialects/base/dialect.py", line 44, in parse
    return p.parse_from_string(string, *args, **kwargs)
  File "/home/christianh/anaconda3/lib/python3.7/site-packages/omi/dialects/base/parser.py", line 69, in parse_from_string
    **(parse_kwargs or {})
  File "/home/christianh/anaconda3/lib/python3.7/site-packages/omi/dialects/oep/parser.py", line 348, in parse
    for field_name in fk.get("fields", [])
  File "/home/christianh/anaconda3/lib/python3.7/site-packages/omi/dialects/oep/parser.py", line 348, in <listcomp>
    for field_name in fk.get("fields", [])
KeyError: None
MGlauer commented 4 years ago

"fields": [null] is not something I would consider valid. But this is debatable. Semantically this means "There is a field and this field is nothing" which is weird.

christian-rli commented 4 years ago

Hmm, that makes sense. I tried to fill out a string that has no foreignKey, but preserving all keys of the metadata json structure. This is not possible however.

When "foreignKeys": [{"fields": [], ... AND When "reference":{ "fields": null, ...} there is a TypeError ('NoneType' object is not iterable).

When "foreignKeys": [{"fields": [], ... AND When "reference":{ "fields": [], ...} there is an Exception (Missing reference in foreign key).

With the option of an existing field in which there is nothing: When "foreignKeys": [{"fields": [null], ... AND When "reference":{ "fields": [null], ...} there is a key Error: None (which is just logical, I suppose)

With everything just null: When "foreignKeys": [{"fields": null, ... AND When "reference":{ "fields": null, ...} there is a TypeError: 'NoneType' object is not iterable (because a list is expected)

Now the way I found around this is to just leave foreignKeys as an empty list "foreignKeys": []. However this eliminates a few json keys from the oemetadata string. I think there is an added value to allowing to preserve those keys even with no foreign keys. The question is how big that value is.

I guess it makes logical sense to have an empty list when there are no foreignKeys. However this does not conform to our approach / our recommendation of just putting null wherever something does not apply. Although, I suppose helper tools could help users steer around this pitfall...

In your opinion @MGlauer , what would a key preserving empty foreignKeys section look like that omi should be able to parse? Or does the concept of an empty nested object not make sense at all?

MGlauer commented 4 years ago

You are right, there is no solution to this problem - and this is intentional. If a list has length > 0 it means that there must be an object in this list. Therefore, if there is an object in the list of foreign keys, there is a foreign key and a foreign key has required fields - e.g. fields and a target table with columns. I am not sure what a foreign key without columns would be. So, yes, such a structure does not make sense

christian-rli commented 4 years ago

OK, thanks for elaborating. I can follow that logic.

From this follows then that strings without a foreign key need to be an empty list like so "foreignKeys": []. Any helper tools should consider this and it should also probably be reflected in the helpful comments section at the bottom of the string. I'll add it to https://github.com/OpenEnergyPlatform/oemetadata/issues/34

Closing this issue and adapting the original string that made me open this issue.