-
Notifications
You must be signed in to change notification settings - Fork 265
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 return servicePath in the entity response #4329
Fix return servicePath in the entity response #4329
Conversation
@fgalan, Please review the PR and let me know if you have any suggestions. |
Thank you for the PR. However, I think this is already covered with existing functionality. Let me elaborate:
What do you think? |
I agree with you. However, I am not aware of that because I am new to orion activity. I'll take the necessary precautions to avoid making the same mistakes in the future. |
Don't worry, it's not your fault :) It's ours, as we have should have detected this and close parent issue before you start working it... Thus, do you agree in closing this PR? |
Yes, I agree. |
Thank you for your understanding! Issue #2877 has been also closed. |
Taking into account the feedback from @kzangeli at #2877 (comment), we are reopening this PR. @chetan-NEC please do .test based on the case @kzangeli is proposing:
Thanks! |
As @kzangeli recommended, I am going to look into it. |
@fgalan, please provide me some advice on how to use wildcards and multiple elements in the fiware-servicepath. |
The functionally is described here: https://github.com/telefonicaid/fiware-orion/blob/master/doc/manuals/orion-api.md#service-path Not sure if this answer your question, as it is very broad... In that case, please come back with more specific questions. |
If I remember correctly, an NGSIv2 entity has only one service path, and this issue is about returning it, right? The service path will be (I assume) a field in the json response payload body, per entity, right next to entity id/type |
Yes, you're correct. The service path should be returned in the entity response, according to this issue. and in this PR, I've added this functionality. |
You should add more .test to cover the wildcards and list cases. For instance:
|
Btw, conflict in CHANGES_NEXT_RELEASE file (trivial to solve) needs to be addressed. |
Thank you for the suggestion. |
@fgalan, In this PR, I've added test cases as you suggested. Please review the PR and let me know if you have any suggestions. |
PR looks fine but the conflict in CHANGES_NEXT_RELEASE has to be solved. At the present moment, this is the only line it should appear in that file:
In addition, I'd suggest to add some test testing the new servicePath built-in attribute in notifictions, using |
@fgalan, I have updated this PR with an additional test case, as you suggested. |
.../cases/2877_return_ServicePath_in_entity_response/servicePath_behave_with_q_and_orderBy.test
Show resolved
Hide resolved
.../cases/2877_return_ServicePath_in_entity_response/servicePath_behave_with_q_and_orderBy.test
Show resolved
Hide resolved
Hello, @fgalan I would like to request you that let's merge the current PR and open a new issue for this suggested functionality, as q and orderBy arguments are new functionalities for the servicePath built-in attribute. I feel this suggestion implementation would take little more effort. |
Builtin attributes should behave as regular attributes and, in this sense, they should properly support Although we could "skip" that support in this PR, that would leave an awful "technical debt" after it (that should be documented, which is also an extra work). Thus, I think it worths the effort to complete the implementation of |
Thank you for your suggestion. I am looking into this thing, as you suggested. |
@fgalan, please review the PR and let me know if you have any suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the contribution!
Fix #2877