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

[v8.0] fix (Transformation): use parameterised query in addTransformation #7894

Conversation

ryuwd
Copy link

@ryuwd ryuwd commented Nov 18, 2024

This is so I am able to submit transformations despite my name having an apostrophe in it. Use a parameterised query in addTransformation to sidestep issues around escaping.

BEGINRELEASENOTES

*Transformation
FIX: Use parameterised query in addTransformation

ENDRELEASENOTES

@DIRACGridBot DIRACGridBot added the alsoTargeting:integration Cherry pick this PR to integration after merge label Nov 18, 2024
@ryuwd ryuwd changed the title [rel-v8r0] fix (Transformation): escape parameters in addTransformation [rel-v8r0] fix (Transformation): escape parameters in addTransformation / support submitting Transformations by people with apostrophes in their name Nov 18, 2024
@ryuwd
Copy link
Author

ryuwd commented Nov 18, 2024

the failing tests are suspicious

LocalRepo/TestCode/DIRAC/tests/Integration/TransformationSystem/Test_Client_Transformation.py::TransformationClientChain::test_addAndRemove FAILED [ 33%]

possible that escapeString may be slow?

@ryuwd ryuwd force-pushed the roneil-make-it-so-i-can-submit-transformations branch from fa08ef4 to 895af75 Compare November 18, 2024 21:16
@ryuwd
Copy link
Author

ryuwd commented Nov 18, 2024

I combed the mysqlclient source to check, but it looks like either %(key)s (with args as a dict) or %s (with args as a tuple) are the intended placeholders regardless of the types of the dict values / tuple values.

cc @chrisburr

@ryuwd ryuwd changed the title [rel-v8r0] fix (Transformation): escape parameters in addTransformation / support submitting Transformations by people with apostrophes in their name [v8.0] fix (Transformation): use parameterised query in addTransformation Nov 18, 2024
@ryuwd ryuwd force-pushed the roneil-make-it-so-i-can-submit-transformations branch from 895af75 to cfc8e73 Compare November 18, 2024 21:44
@ryuwd ryuwd force-pushed the roneil-make-it-so-i-can-submit-transformations branch from cfc8e73 to caab09b Compare November 18, 2024 21:45
@ryuwd ryuwd marked this pull request as draft November 18, 2024 21:58
@ryuwd ryuwd marked this pull request as ready for review November 19, 2024 12:07
@fstagni fstagni merged commit 1054749 into DIRACGrid:rel-v8r0 Nov 20, 2024
26 checks passed
@DIRACGridBot DIRACGridBot added sweep:done All sweeping actions have been done for this PR sweep:failed Sweeping failed and needs manual intervention labels Nov 20, 2024
@DIRACGridBot
Copy link

Sweep summary

Sweep ran in https://github.com/DIRACGrid/DIRAC/actions/runs/11929703771

Failed:

  • integration
    cherry-pick 1054749 into integration failed
    check merge conflicts on a local copy of this repository
    git fetch upstream
    git checkout upstream/integration -b cherry-pick-2-1054749d2-integration
    git cherry-pick -x -m 1 1054749d2
    # Fix the conflicts
    git cherry-pick --continue
    git commit --amend -m 'sweep: #7894 fix (Transformation): use parameterised query in addTransformation' --author=''
    git push -u origin cherry-pick-2-1054749d2-integration
    
    # If you have the GitHub CLI installed the PR can be made with
    gh pr create \
         --label 'sweep:from rel-v8r0' \
         --base integration \
         --repo DIRACGrid/DIRAC \
         --title '[sweep:integration] fix (Transformation): use parameterised query in addTransformation' \
         --body 'Sweep #7894 `fix (Transformation): use parameterised query in addTransformation` to `integration`.
    
    Adding original author @ryuwd as watcher.
    
    BEGINRELEASENOTES
    
    *Transformation
    FIX: Use parameterised query in addTransformation
    
    ENDRELEASENOTES
    Closes #7900'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alsoTargeting:integration Cherry pick this PR to integration after merge sweep:done All sweeping actions have been done for this PR sweep:failed Sweeping failed and needs manual intervention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants