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] Use jq to handle GraphDB credentials payloads with special characters #109

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

alyssadai
Copy link
Contributor

@alyssadai alyssadai commented Feb 11, 2025

Changes proposed in this pull request:

  • Install jq in GraphDB container to construct JSON payloads for curl requests for setting graph credentials
    • most robust method of ensuring that special characters in the JSON body are escaped
    • other methods tested:
      • reading file contents directly into JSON body (instead of using environment variable) in curl request, using cat or heredoc ❌
      • constructing whole JSON payload 'manually' by first writing to a temporary payload.json, and then sending the temp file in curl ❌
      • formatting JSON payload in Python - not installed ❌
  • Manually tested updated recipe using passwords containing \, ", $

Checklist

This section is for the PR reviewer

  • PR has an interpretable title with a prefix ([ENH], [FIX], [REF], [TST], [CI], [MNT], [INF], [MODEL], [DOC]) (see our Contributing Guidelines for more info)
  • PR has a label for the release changelog or skip-release (to be applied by maintainers only)
  • PR links to GitHub issue with mention Closes #XXXX
  • Tests pass
  • Checks pass

For new features:

  • Tests have been added

For bug fixes:

  • There is at least one test that would fail under the original bug conditions.

@alyssadai alyssadai added release Create a release when this PR is merged pr-bug-fix Bug fix, will increment patch version when merged (0.0.+1) labels Feb 11, 2025
@alyssadai alyssadai changed the title Use jq to handle GraphDB credentials payloads with special characters [FIX] Use jq to handle GraphDB credentials payloads with special characters Feb 11, 2025
@alyssadai alyssadai changed the title [FIX] Use jq to handle GraphDB credentials payloads with special characters [FIX] Use jq to handle GraphDB payloads with special characters Feb 11, 2025
@alyssadai alyssadai changed the title [FIX] Use jq to handle GraphDB payloads with special characters [FIX] Use jq to handle GraphDB credentials payloads with special characters Feb 11, 2025
@alyssadai alyssadai marked this pull request as ready for review February 11, 2025 18:56
@surchs surchs self-requested a review February 11, 2025 20:46
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

Looks good @alyssadai, and thanks for addressing!

This is good to go, but could you already open an issue to revisit the "install jq every time" solution, e.g. by making our own image on top of the graphDB one. That might also be a good place to see if we shouldn't upgrade the graphDB image tag we're pulling.

🧑‍🍳

@alyssadai alyssadai merged commit aff9915 into main Feb 11, 2025
2 checks passed
@alyssadai alyssadai deleted the escape-password branch February 11, 2025 21:33
Copy link
Contributor

🚀 PR was released in v0.4.2 🚀

@neurobagel-bot neurobagel-bot bot added the released This issue/pull request has been released. label Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bug-fix Bug fix, will increment patch version when merged (0.0.+1) release Create a release when this PR is merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Graph passwords with some special characters are not escaped
2 participants