Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Fix ServiceDecorator parsing in oob record handling #2910

Merged
merged 5 commits into from
Apr 24, 2024

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented Apr 23, 2024

Fixes find_oob_target_for_outbound_message and find_oob_record_for_inbound_message in OobMessageProcessor

Context:
Attempting to upgrade to 0.12 in https://github.com/didx-xyz/aries-cloudapi-python

Our existing OOB test workflows would raise an error from the following line in find_oob_record_for_inbound_message:

        their_service = (
            cast(
                ServiceDecorator,
                ServiceDecorator.deserialize(oob_record.their_service),
            )
            if oob_record.their_service
            else None
        )

The error:

Traceback (most recent call last):
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/messaging/models/base.py", line 196, in deserialize
    schema.loads(obj) if isinstance(obj, str) else schema.load(obj),
  File "/home/aries/.local/lib/python3.9/site-packages/marshmallow/schema.py", line 723, in load
    return self._do_load(
  File "/home/aries/.local/lib/python3.9/site-packages/marshmallow/schema.py", line 910, in _do_load
    raise exc
marshmallow.exceptions.ValidationError: {'_schema': ['Invalid input type.']

I noticed that according to the compiler, oob_record.their_service was already of type ServiceDecorator ... so cast(ServiceDecorator, ServiceDecorator.deserialize(... is especially code-smelly!

Resolving that to simply be their_service = oob_record.their_service led to the following exception:

...
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/admin/server.py", line 148, in send_outbound
    return await self._send(profile, message)
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/core/conductor.py", line 647, in outbound_message_router
    status: OutboundSendStatus = await self._outbound_message_router(
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/core/conductor.py", line 674, in _outbound_message_router
    return await self.queue_outbound(profile, outbound, inbound)
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/core/conductor.py", line 722, in queue_outbound
    outbound.target = await self.dispatcher.run_task(
  File "/usr/local/lib/python3.9/asyncio/futures.py", line 284, in __await__
    yield self  # This tells Task to wait for completion.
  File "/usr/local/lib/python3.9/asyncio/tasks.py", line 328, in __wakeup
    future.result()
  File "/usr/local/lib/python3.9/asyncio/futures.py", line 201, in result
    raise self._exception
  File "/usr/local/lib/python3.9/asyncio/tasks.py", line 256, in __step
    result = coro.send(None)
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/core/oob_processor.py", line 110, in find_oob_target_for_outbound_message
    outbound_message.payload = json.dumps(message)
...
TypeError: Object of type ServiceDecorator is not JSON serializable

The above was fixed by adding .serialize() to message["~service"] = oob_record.our_service.serialize()
Tada! With this modification, our previous OOB test workflow is passing again.

This appears to come from some type inconsistencies ... where services are references as either dictionaries or ServiceDecorator types.

Some tests were failing due to mocking their/our_service as dictionaries. Resolved that by using the correct model.

Signed-off-by: ff137 <ff137@proton.me>
@swcurran swcurran requested a review from dbluhm April 23, 2024 20:53
Signed-off-by: ff137 <ff137@proton.me>
@ff137
Copy link
Contributor Author

ff137 commented Apr 23, 2024

As far as I can tell, the bug may have been introduced with the refactoring in #2862: 7f5eae7, where _their_service/_our_service is introduced with a SerDe type. But I'm not sure, may be another change

@ff137
Copy link
Contributor Author

ff137 commented Apr 23, 2024

@dbluhm I notice now that #2908 already started this initiative! Your checks and exception handling is better there, so I'll pull in your additions here, because this one includes fixes to the tests. Can also do the same and pull test fixes to your PR if they make sense

…b-service-parse

Signed-off-by: ff137 <ff137@proton.me>
Signed-off-by: ff137 <ff137@proton.me>
Copy link

sonarcloud bot commented Apr 23, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@ff137 ff137 changed the title 🐛 Fix ServiceDecorator references in finding oob target 🐛 Fix ServiceDecorator parsing in oob record handling Apr 23, 2024
Copy link
Member

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Beat me to it 🙂

@dbluhm dbluhm merged commit c2b4703 into hyperledger:main Apr 24, 2024
8 checks passed
@ff137 ff137 deleted the fix/find_oob_target branch April 24, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants