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

Added depth parameter to win_uri ConvertTo-Json call #587

Merged
merged 7 commits into from
Mar 6, 2024

Conversation

thomasriera-akkodis
Copy link
Contributor

Added depth parameter to win_uri ConvertTo-Json call

SUMMARY

When using the win_uri module to send a json object, win_uri uses the ConvertTo-Json method.

This method has a currently unused parameter called Depth which represents the maximum depth of nested children to parse.
(https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.utility/convertto-json?view=powershell-7.4#-depth)

This parameter's default value is 2, so the method does not correctly convert json objects with a higher depth.

I set it to a default of 20 which should be enough for most cases, or at least more useful than the default of 2.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

win_uri.ps1

ADDITIONAL INFORMATION

To reproduce the bug : send a json object with more than 2 levels of nesting using win_uri. The method will seem to work but the sent object will be incomplete.

With this change, the module can correctly send json objects with up to 20 levels of nesting.

Added depth parameter to win_uri ConvertTo-Json call
Copy link
Collaborator

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR here, I've added a tiny suggestion that adds a comment explaining why it is ok to add the custom depth.

For us to merge this PR we do need 2 extra things here

  • A changelog fragment
  • Some tests for this scenario

The changelog fragment is documented under https://docs.ansible.com/ansible/latest/community/development_process.html#creating-a-changelog-fragment, this feature would come under minor_changes and would be placed in https://github.com/ansible-collections/ansible.windows/tree/main/changelogs/fragments

The tests for win_uri are located at https://github.com/ansible-collections/ansible.windows/blob/main/tests/integration/targets/win_uri/tasks/main.yml. You would probably want something similar to

- name: send JSON body with dict type
win_uri:
url: https://{{httpbin_host}}/post
method: POST
body:
foo: bar
list:
- 1
- 2
dict:
foo: bar
headers:
'Content-Type': 'text/json'
return_content: yes
register: json_as_dict
just with a more nested object structure as a new test.

Please let me know if you need any help with the above and I'm happy to point you in the right direction.

plugins/modules/win_uri.ps1 Show resolved Hide resolved
Added test for sending json objects with many levels of nesting in win_uri
Removed erroneously duplicated test
Syntax fixes
Added assertion for nested json win_uri test case
@jborean93
Copy link
Collaborator

@thomasriera-akkodis do you mind if I try and push some changes to get the tests working, I'm hoping to include this in the next release of this collection which I'm planning on doing soon.

Fixed value of assertion json target object
@thomasriera-akkodis
Copy link
Contributor Author

@jborean93

I think everything should be okay now. Happy to see this change coming soon, I'll be able to remove my dirty workaround from my servers.

Copy link
Collaborator

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, the changes look great!

@jborean93 jborean93 merged commit b02f66c into ansible-collections:main Mar 6, 2024
24 checks passed
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