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

Change the type of config data between client and gateway #1285

Merged
merged 6 commits into from
Apr 17, 2024

Conversation

akihikokuroda
Copy link
Collaborator

@akihikokuroda akihikokuroda commented Apr 12, 2024

Summary

Fix mismatch between the data type of the config in the post data and the data type in the data model
Fix #1268

Details and comments

The mismatch made some special handling for the config data in the serializer.

Copy link
Member

@Tansito Tansito left a comment

Choose a reason for hiding this comment

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

I think these changes are good but to avoid to use QueryDict in the request we need to specify that a request is a json and that's changing in requests the field data by json from what I could read here:

https://requests.readthedocs.io/en/latest/user/quickstart/#more-complicated-post-requests

Please note that the above code will NOT add the Content-Type header (so in particular it will NOT set it to application/json).

If you need that header set and you don’t want to encode the dict yourself, you can also pass it directly using the json parameter (added in version 2.4.2) and it will be encoded automatically:

url = 'https://api.github.com/some/endpoint'
payload = {'some': 'data'}
r = requests.post(url, json=payload)
Note, the json parameter is ignored if either data or files is passed.

@akihikokuroda
Copy link
Collaborator Author

@Tansito I misunderstood the issue. I think this PR is still valid improvement. I'll change the description of this PR and disassociates from the issue #1268.

@akihikokuroda akihikokuroda dismissed Tansito’s stale review April 15, 2024 20:04

This PR is separated from the issue now.

gateway/api/serializers.py Outdated Show resolved Hide resolved
Copy link
Member

@Tansito Tansito left a comment

Choose a reason for hiding this comment

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

LGTM @akihikokuroda , should we open issues to do the same for arguments and env_vars?

@akihikokuroda
Copy link
Collaborator Author

Thanks! No, we don't unless we want to change the field type of in the models. I don't know if it's a good thing to do changing the field type at this moment. WDYT?

@Tansito
Copy link
Member

Tansito commented Apr 17, 2024

Mmm... Maybe we can open a discussion for this because I don't want to add too many comments in the PR. My proposal would be something like:

  • Remove arguments from Job too and send them as standard JSON
  • Send env_vars as standard JSON because at the end what we are storing is the encryption of arguments + env_vars
  • If this option doesn't like send arguments as standard JSON and encrypt them as they were env_vars

@akihikokuroda
Copy link
Collaborator Author

@Tansito Yes, new issue or discussion is good. Would you create one?

@akihikokuroda akihikokuroda merged commit 9bab4a9 into Qiskit:main Apr 17, 2024
12 checks passed
@Tansito Tansito mentioned this pull request Apr 17, 2024
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.

Use JSON standar for all our calls to the gateway
2 participants