added ql version in metadata

[daminichopra]

Proposed changes

Stored ql_version in metadata

Types of changes

What types of changes does your code introduce to the project: Put an x in the boxes that apply

• [ x ] Bugfix (non-breaking change which fixes an issue)
• [ x ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

• [ x ] I have read the CONTRIBUTING doc
• [ x ] I have signed the CLA
• [ ] I have added tests that prove my fix is effective or that my feature works
• [ x ] I have updated the [RELEASE_NOTES][RELEASE_NOTES.md]
• [ ] I have added necessary documentation (if appropriate)
• [ ] Any dependent changes have been merged and published in downstream modules

Further comments

Fix for Issue

[github-actions[bot]]

CLA Assistant Lite bot All contributors have signed the CLA ✍️

[daminichopra]

Hi @chicco785 @c0c0n3 , I have done some changes in code test case are passing but still autopep8 is failing not able to get exact cause why it is failing.

[c0c0n3]

hi @daminichopra :-)

I'm not sure why autopep is failing, I looked at the logs but couldn't find any useful clues. We'll look into this. In the meantime, if you could please review my suggestion below and implement it if you agree with the proposed changes.

We've got to store the version number in a single place to avoid grief down the line. So I suggest we add a src/_version.py file with the following content

# Most recent official release
__version_info__ = ('0', '8', '3')
__version__ = '.'.join(__version_info__)

# Current dev version for the next release
__dev_version__ = '0.9.0-dev'

See also this SO question about app versions

• https://stackoverflow.com/questions/458550

Once this module is in place, replace all hard-coded version numbers scattered around the code with the constants defined above. For example, here's what the reporter.version module should look like

from _version import __dev_version__

def version():
    return {
        'version': __dev_version__
    }

Same applies to the module's test. On the other hand, you should use the __version__ constant in the new code you've implemented for this PR.

Thank you so much!

[daminichopra]

hi @daminichopra :-)

I'm not sure why autopep is failing, I looked at the logs but couldn't find any useful clues. We'll look into this. In the meantime, if you could please review my suggestion below and implement it if you agree with the proposed changes.

We've got to store the version number in a single place to avoid grief down the line. So I suggest we add a src/_version.py file with the following content

# Most recent official release
__version_info__ = ('0', '8', '3')
__version__ = '.'.join(__version_info__)

# Current dev version for the next release
__dev_version__ = '0.9.0-dev'

See also this SO question about app versions

• https://stackoverflow.com/questions/458550

Once this module is in place, replace all hard-coded version numbers scattered around the code with the constants defined above. For example, here's what the reporter.version module should look like

from _version import __dev_version__

def version():
    return {
        'version': __dev_version__
    }

Same applies to the module's test. On the other hand, you should use the __version__ constant in the new code you've implemented for this PR.

Thank you so much!

@c0c0n3 , Incorporated comments suggested and updated PR.

[c0c0n3]

@daminichopra thank you sooo much that's great. We're going to need some tests though :-) But before you do that, let's wait for @chicco785 to comment on this PR since I remember him mentioning he had something different in mind when he wrote #591?

[daminichopra]

@daminichopra thank you sooo much that's great. We're going to need some tests though :-) But before you do that, let's wait for @chicco785 to comment on this PR since I remember him mentioning he had something different in mind when he wrote #591?

@c0c0n3 , Sure

[c0c0n3]

@daminichopra thanks for this contrib, we just need you to add some tests to check that the QL version gets written to the meta table...Any chance you could you do that? Thanks alot!!!

[pooja1pathak]

@c0c0n3 @chicco785 I have updated this PR. Please let me know in case any other change is required to merge this PR.

[pooja1pathak]

@c0c0n3 @chicco785 I have updated the PR. If you can spare sometime for review and let me know in case any further change is required.

[c0c0n3]

@daminichopra @pooja1pathak thank you for the reminder. We've been busy w/ other projects and this PR fell through the cracks. Soooo sorry about that. Anyhoo, we've just checked out your branch and had a look at the code which is mostly fine, but we'd like to change where the version gets stored in the DB.

At the moment it ends up being stored as an NGSI attribute in the meta table. E.g. with two tables et0 and et1 this is what I get

table_name | entity_attrs
-------------------------
"et0" | {"ql_version":["0.8.3","Text"],"attr_bool":["attr_bool","Boolean"],"entity_type":["type","Text"],"time_index":["time_index","DateTime"],"attr_geo":["attr_geo","geo:json"],"attr_time":["attr_time","DateTime"],"attr_str":["attr_str","Text"],"entity_id":["id","Text"],"attr_float":["attr_float","Number"]}
"et1" | {"ql_version":["0.8.3","Text"],"attr_bool":["attr_bool","Boolean"],"entity_type":["type","Text"],"time_index":["time_index","DateTime"],"attr_geo":["attr_geo","geo:json"],"attr_time":["attr_time","DateTime"],"attr_str":["attr_str","Text"],"entity_id":["id","Text"],"attr_float":["attr_float","Number"]}

Ideally the version should be stored in a separate column since it's an property of the table, not of the NGSI entities? So the table should probably look something like

table_name | version | entity_attrs
-----------------------------------
"et0" | "0.8.3" | {"attr_bool":["attr_bool","Boolean"],"entity_type":["type","Text"],"time_index":["time_index","DateTime"],"attr_geo":["attr_geo","geo:json"],"attr_time":["attr_time","DateTime"],"attr_str":["attr_str","Text"],"entity_id":["id","Text"],"attr_float":["attr_float","Number"]}
"et1" | "0.8.3" | {"attr_bool":["attr_bool","Boolean"],"entity_type":["type","Text"],"time_index":["time_index","DateTime"],"attr_geo":["attr_geo","geo:json"],"attr_time":["attr_time","DateTime"],"attr_str":["attr_str","Text"],"entity_id":["id","Text"],"attr_float":["attr_float","Number"]}

But I'll let @chicco785 comment on this if he thinks it's best to have the version column stored somewhere else rather than an additional meta column.

[c0c0n3]

@daminichopra @pooja1pathak thank you for the reminder. We've been busy w/ other projects and this PR fell through the cracks. Soooo sorry about that. Anyhoo, we've just checked out your branch and had a look at the code which is mostly fine, but we'd like to change where the version gets stored in the DB.

At the moment it ends up being stored as an NGSI attribute in the meta table. E.g. with two tables et0 and et1 this is what I get

table_name | entity_attrs
-------------------------
"et0" | {"ql_version":["0.8.3","Text"],"attr_bool":["attr_bool","Boolean"],"entity_type":["type","Text"],"time_index":["time_index","DateTime"],"attr_geo":["attr_geo","geo:json"],"attr_time":["attr_time","DateTime"],"attr_str":["attr_str","Text"],"entity_id":["id","Text"],"attr_float":["attr_float","Number"]}
"et1" | {"ql_version":["0.8.3","Text"],"attr_bool":["attr_bool","Boolean"],"entity_type":["type","Text"],"time_index":["time_index","DateTime"],"attr_geo":["attr_geo","geo:json"],"attr_time":["attr_time","DateTime"],"attr_str":["attr_str","Text"],"entity_id":["id","Text"],"attr_float":["attr_float","Number"]}

Ideally the version should be stored in a separate column since it's an property of the table, not of the NGSI entities? So the table should probably look something like

table_name | version | entity_attrs
-----------------------------------
"et0" | "0.8.3" | {"attr_bool":["attr_bool","Boolean"],"entity_type":["type","Text"],"time_index":["time_index","DateTime"],"attr_geo":["attr_geo","geo:json"],"attr_time":["attr_time","DateTime"],"attr_str":["attr_str","Text"],"entity_id":["id","Text"],"attr_float":["attr_float","Number"]}
"et1" | "0.8.3" | {"attr_bool":["attr_bool","Boolean"],"entity_type":["type","Text"],"time_index":["time_index","DateTime"],"attr_geo":["attr_geo","geo:json"],"attr_time":["attr_time","DateTime"],"attr_str":["attr_str","Text"],"entity_id":["id","Text"],"attr_float":["attr_float","Number"]}

But I'll let @chicco785 comment on this if he thinks it's best to have the version column stored somewhere else rather than an additional meta column.

[pooja1pathak]

@c0c0n3 , I have updated the PR as per your suggestion. Kindly review and let me know in case any other change is required.

[c0c0n3]

@pooja1pathak thanks so much for accommodating this refactoring. One thing that I'm still not sure about is what's going to happen in the case of upgrading QuantumLeap from a previous version.

In that case, we should assume there's already a QuantumLeap DB in place, either Crate or Timescale, with a metadata table in it. Now that table, having being created by a QL version released before this PR, won't have the version column in it. I've got a sneaking suspicion our current approach won't work in this scenario?

The _create_metadata_table methods of both translators (Crate & Timescale) issue a create table if not exists command where the version column gets slotted in. But like I said, in an upgrade scenario, the metadata table will be there already, so the create table if not exists command will do nothing, which means the version column won't be created. Now when QL tries inserting the version number later, I'd guess the insert statement should crash b/c the version column isn't part of the metadata table?

[pooja1pathak]

@c0c0n3 I have checked above scenario, Please find below observations:

1. When I create an entity from master branch of the original repo, in metadata table only two columns are created i.e.,

table_name	entity_attrs

2. When I send notification for the same entity to daminichopra:Issue519, then notification is processed successfully but this also only two columns table_name | entity_attrs in metadata table. version is not added in the table.
3. When I send notification with different type, fiware-service and fiware-servicePath, then version is added in metadata table with current version value. For the previous value it inserts NULL.

As per my observation, the this implementation will not cause any issue with the current code.

Please suggest if any other improvement is needed from my side or we can merge this PR.

[c0c0n3]

@pooja1pathak excellent work! Can you test with Timescale as a backend too? Just for peace of mind since Crate might work b/c of the dynamic table policy---or maybe there's something else which makes it work. The test should be the same of what you've done w/ Crate:

1. Start Timescale
2. Start QL 0.8.3 w/ Timescale as a backend
3. Insert an entity of type e for service s and service path p---where e, s and p are whatever you like
4. Verify there's no version column in the meta table
5. Stop QL, but keep Timescale up and running---this is important if you're using Docker Compose since if you stop the service and the DB directory isn't mounted on a volume, the old DB will be gone when you restart Timescale
6. Start QL w/ the code from this branch
7. Repeat step (3)---entity type, service and service path should be exactly the same as in (3)
8. Verify there's a version column with a value of 0.8.3.

Thank you so much!

[pooja1pathak]

@c0c0n3 I have checked timescale as per your suggestion. Timescale's behavior is different from crateDB.

metadata table is created for the first time with QL 0.8.3, but when I replace QL with branch daminichopra:Issue#519 and then perform the notify operation for same and different entity type and fiware-service.

In both the cases metadata table is not updated.

I am looking into it to find the real cause and will update the PR if I find any solution.