add_session_metadata api is broken

[klimburg]

The add_session_metadata api results in a 500.

There are two separate issues in the implementation as it exists today. Session.metadata_objects does not exist but rather Session.metadata_items is the correct attribute and then the key doesnt get set. Its unclear whether add_session_metadata is intended to add one item at a time or if the metadata dict is intended to set multiple items one for each key. Depending on the intent either of the following implementation works.

One Item per call

@API
def add_session_metadata(id: int, key: str, metadata: Any):
    try:
        session = Session.query.filter(Session.id == id).one()
        session.metadata_items.append(
            SessionMetadata(key=key, metadata_item=metadata))
    except NoResultFound:
        abort(requests.codes.not_found)
    db.session.commit()

Multiple Items per call

@API
def add_session_metadata(id: int, metadata: dict):
    try:
        session = Session.query.filter(Session.id == id).one()
        for key, value in metadata.items():
            session.metadata_items.append(
                SessionMetadata(key=key, metadata_item=value))
    except NoResultFound:
        abort(requests.codes.not_found)
    db.session.commit()

Personally I prefer the multiple items per call approach.

Finally I suspect add_test_metadata has a similar issue but I havent tested it yet. Just looking at the source I believe it should be test.metadatas and not test.metadata_objects and similar with the missing key parameter to the TestMetadata constructor.

@vmalloc If i submit fixes will they be taken? It seems like development has really slowed down.

[vmalloc]

@klimburg yes! I might take a short while to get to it because I'm a bit busy with other projects nowadays but fixes and PRs are always welcome!

Having said that, it seems like the CI is currently broken, which is another topic I need to tackle when I find the time for it. Assuming the fix makes sense and is tested on your side, I'm open to accepting it without the CI accepting it for the time being.

Thanks a lot for the help and patience!

[klimburg]

Great! This project really meets a lot of my needs so I don't mind contributing fixes and improvements as its still much less effort than rolling my own solution from scratch. Really appreciate all your efforts and understand being busy with other projects!

Cheers!

[klimburg]

I dug into this today some more, it seems that add_test_metadata and add_session_metadata are not actually necessary. As the both the backslash Session and Test objects inherit from MetadataHolder which use/implement the set_metadata and set_metadata_dict apis.

Therefore it seems like the correct thing to do would be to delete these API endpoints unless they are left behind for backwards compatibility which was unintentionally broken.

As a test author I can do the following to add session metadata or test metadata with the backslash plugin today

backslash = slash.plugins.manager.get_plugin("backslash")
backslash.session.set_metadata_dict({"abc": 123})
backslash.current_test.set_metadata("foo", "bar")

[vmalloc]

I think backward compatibility is important here. The API isn't really versioned, and I have no clear visibility over the current user base. Perhaps do this as a part of a major version bump so that people are aware of the differences (also a release notes explaining it)

[klimburg]

Ok once we get the CI working I’ll do that.