huserben / TfsExtensions

Extensions for TFS 2015+ such as custom Widgets (require TFS 2017) and Build Tasks
MIT License
45 stars 22 forks source link

Special Characters are not escaped in JSON that is sent to Server #47

Closed jfinnegan3 closed 6 years ago

jfinnegan3 commented 6 years ago

Hello I'm using your tool as documented, however I am receiving a: " task result:Failed Could not Trigger build. "

I'm fairly confident that my authentication is correct, as I was receiving authentication errors until I enabled OAuth and allowed use of the token in scripts as you recommened in another issue forum.

The console tells me: " Queue New Build for definition This request expects an object in the request body, but the supplied data could not be deserialized. "

Is there any reason that this may be caused?

huserben commented 6 years ago

Hi @jfinnegan3

could you provide me de complete logs of the task?

jfinnegan3 commented 6 years ago

Hello @huserben, here is the excerpt from the appropriate task:

##[section]Starting: Trigger Build Tasks
==========================================
Task: Trigger Build
Description: ...
Version: 2.4.5
Author: Sir Benjamin Huser
Help: 
==========================================
Using current Team Project Url
Path to Server: <path>
Trying to fetch authentication token from system...
Using OAuth Access Token
Build shall be triggered for same user that triggered current build: <dev_name>
Source Version: <source_version>
Triggered Build will use the same source version: <source_version>
Using same branch as source version: <changeset_name>
Will trigger build with following demands:
DotNetFramework
Build Queue was specified as string <queue_name> - trying to fetch Queue ID for the queue...
Found id of queue <queue_name>: <queue_id>
Will trigger build in following agent queue: <queue_id>
Will trigger build with following parameters: \"BuildPlatform\": \"x64\", \"system.debug\": \"false\", \"BuildConfiguration\": \"Development\", \"system.collectionId\": \"<guid>\", \"system.teamProject\": \"<project_name>"
Queue new Build for definition <definition_name>
This request expects an object in the request body, but the supplied data could not be deserialized.
##[error]Could not Trigger build. See console for more Information.
##[section]Finishing: Trigger Build Tasks

This is the complete logging that I am seeing for the Trigger Build Tasks task. The Demands and Build parameters were not always there, I was experimenting - trying to get it to work - as I thought the deserialization may not have been working because I wasn't supplying an expected variable.

huserben commented 6 years ago

Hi

thanks for the log. My first guess when looking at the logs and the error would be something is wrong with the build parameters. Could it be that you missed a backslash at the very last build parameter?

\"BuildPlatform\": \"x64\", \"system.debug\": \"false\", \"BuildConfiguration\": \"Development\", \"system.collectionId\": \"<guid>\", \"system.teamProject\": \"<project_name>"

jfinnegan3 commented 6 years ago

The missing backslash is a mistake I made when transferring the logs. The backslash is present in my parameters. I just removed all of the demands and build parameters and received the same error. The log of this build attempt is identical minus the demands and parameters lines. When I re-enter the build parameters and intentionally remove a backslash, I receive the identical error, so I'd suspect that the error results from either processing before those build parameters or, as you stated, processing of them.

huserben commented 6 years ago

ok, i guess it would have been to easy if it would have just been a typo :-)

Could you try to disable all "special" settings. So not specifying demands and no parameters, but as well do not use the same source and same branch. Just to try to pinpoint the issue, as so far I think i have not yet come across this specific error

jfinnegan3 commented 6 years ago

ok @huserben, this was a successful trigger - but necessary settings are unchecked :P.

True if the build to be triggered is defined... = true Name of build to trigger = Queue Build for original user = false Ignore SSL cert errors = false

use current changeset = false use same branch = false defines branch = empty wait until triggered builds = false store triggered build ids = false demands = empty define queue = define params = empty

These are my current configurations, and as I said, the build was successfully triggered. Unfortunately, it's very important that we use the same changeset and branch, and that we wait for the triggered builds to finish.

huserben commented 6 years ago

Ok, but at least we know now that it is in general working. Can you just enable now the same changeset and same branch option? And in case it then starts to fail, just use either one on its own to see if there might be an issue if they are used in combination?

What environment do you have? TFS or VSTS? Are you using Git or TFVC as Version Control System?

jfinnegan3 commented 6 years ago

so, this is interesting. I kept same changeset and same branch false. I enabled wait until triggered builds are finished with 10 second check interval - and fail if tasks were not successful. This failed.

ok, here is the setup: (same changeset, same branch, wait for build and fail if unsuccessful) (0, 0, 1) = fail (0, 1, 0) = fail as requested - (1, 1, 0) = fail (1, 0, 0) = success

let me know if you'd like me to try any other combinations.

jfinnegan3 commented 6 years ago

I'm working with TFS and using TFVC.

huserben commented 6 years ago

hmm for the first use case you described where it fails when you wait, was the error in the logs the same? Waiting for a build really has no impact in triggering the build itself :-/

I'm now not entirely sure if the "Same Source Branch" is really usable for TFVC. As there the branches are actually using a different path (unlike in Git) i think this might cause the issue. So in TFVC using the same source version should be "enough". (I did so far not put so much thought into the different branch for TFVC, but if it turns out to be like this I definetly need to update the documentation and maybe log some warnings if it is checked anyway)

jfinnegan3 commented 6 years ago

I apologize, I just tried it with waiting enabled and the other 2 not. It was successful - I may not have saved. Here is the updated: (same changeset, same branch, wait for build and fail if unsuccessful) (0, 0, 1) = success (0, 1, 0) = fail as requested - (1, 1, 0) = fail (1, 0, 0) = success

jfinnegan3 commented 6 years ago

and now I just tried (1, 0, 1) and both builds are triggered and working successfully.

huserben commented 6 years ago

ok, I thik we are circling in on the issue.

So it seems using same branch creates problems. Question is why. I just quickly tried in my setup and it works with the configuration (1,1, x). In the log output where we fetch the current branch, does it say: "$/\" ? And if so, could it be that there are some special character in the name?

jfinnegan3 commented 6 years ago

Hello @huserben,

I've run into another issue. As it seems using TFS and TFVC, I need the option use same source branch enabled. As I have it running now, with just use current changeset enabled, it is not building or testing my changeset in the triggered build.

Here is what I did: I coded 2 tests that asserted true = false into the test cases that would be run in each of the triggered builds. I then ran the origin build with the "triggered builds use current changeset" enabled & "use source branch" disabled (obviously). It triggered the 2 builds and they built and tested successfully. My bad tests then got checked-in. I tried to check in a simple comment, and the triggered tasks failed because the bad tests were run.

Behind the scenes, I believe TFS is creating a merged branch of master and the changeset the original build. If I have the "use source branch setting disabled, then that merged branch doesn't get passed to the triggered builds.

huserben commented 6 years ago

Hi

so do I get it correct, that you have a Gated Check-In that should trigger another build and wait for its completion and then continue?

So Build A (Gated Check-In) triggeres Build B (should run with the same sources as A) and A then waits for completion of B and if it was successful it should check the sources in, otherwise the check-in is disabled?

Could you check the "Get Sources" part of Build B and check whether you find the name of your changeset/shelveset there somewhere?

huserben commented 6 years ago

So i know quickly checked again and it seems that Shelvesets are interpreted as "Branch". That explains why Build B for you is not "unshelving" your changes (working under the assumption that you work with shelvesets here via Gated Check-In). This means you need to enable both the options, which brings us back to the original problem.

Could you queue a new build of yours again and set the system.debug option to true. If possible queue it with a Shelveset that you have. Then It would be good if you could provide me the detailed logs. Especially I would be interested of course in the part where the branch name is extracted (most of the other specifics of your environment I don't need).. If you don't want to expose the shelveset name here I could as well give you my mail address where you could send it.

jfinnegan3 commented 6 years ago

Ok, I believe that I know what the problem is:

` ...

[debug]useSameBranch=true

[debug]branchToUse=null

... `

This is with use same source branch and same source changeset enabled

huserben commented 6 years ago

Hi now that's actually correct, the "branchToUse" is the textbox that you could fill in yourself if you deselect "useSameBranch". Internally if specified to use the same branch we will get the info from the environment variable set by TFS to the build agent.

jfinnegan3 commented 6 years ago

so an overview of the process would be this:

Build A is a gated check-in. Build A has one step, it triggers Build B and Build C simultaneously. Build A waits for Build B and Build C to successfully complete (build and test) and fails if either fails. Upon success of Build B and Build C, Build A Trigger task succeeds, and the changeset is checked in.

huserben commented 6 years ago

yeah ok, then we definetly need the "use same branch" option. So if you could provide me the full logs with debug option set to true I could try to spot what might be wrong. In my use case it worked just fine with both options enabled when triggering a build for a shelveset :-/

jfinnegan3 commented 6 years ago

Hey @huserben,

for your tests, do semi-colons in the branch name break your tests? how about a backslash? What does your branch name look like? ie the line: Using same branch as source version: xxxxxxxxxxx

Thanks for checking.

jfinnegan3 commented 6 years ago

I so discovered the root of the problem, and it is a backslash in the branch name ($(Build.SourceBranchName)). I wrote a script to escape the backslash, store that string in a variable and pass it on to the triggered builds within the Build Parameters step, everything is working as expected now. Thanks for the help!

huserben commented 6 years ago

Hi

ok very good. As soon as I have time I will have a look and make the parsing of it more robust so it will work out of the box. I will keep this issue open until this is done.

Thanks for your investigation and your feedback.

huserben commented 6 years ago

Hi @jfinnegan3

Quick question. How did you manage to create a shelveset with a backslash in the first place? I tried now locally via Visual Studio, but it won't let me :-): grafik

I was able to locally reproduce the issue but I wanted to create such a shelveset to have a real test on the TFS with it...

Edit: So I just checked the following page: https://docs.microsoft.com/en-us/vsts/collaborate/naming-restrictions According to this the shelveset cannot contain these special characters...

jfinnegan3 commented 6 years ago

This is a good point. It is not the shelveset name that contains the backslash, but the branch name.

huserben commented 6 years ago

But the "Branch" is actually the shelveset in a TFVC scneario (see my logs): grafik

Or otherwise I'm really confusing now something or don't get your use-case :-/

jfinnegan3 commented 6 years ago

Well, I'm meaning the Build.SourceBranchName contains the problematic, non-escaped character, a backslash. In my use-case, when I have the "use same branch" setting, and that variable is put into JSON format, I receive that JSON deserialization error. I have a work-around that works for me as I have detailed. Personally, I would consider changes for my use-case very low-priority or even non-issue.

If other people are in my boat, they cannot use the "use same branch" setting, and will have to escape the bad characters in the Build.SourceBranchName variable, and pass it on to the triggered builds.

Thanks for an excellent tool, it works like a charm. And thanks for great customer service!

huserben commented 6 years ago

Ok. Well I will still look at possible improvements because at the moment nothing is escaped. So if for whatever reason you have other fields that are submitted via the JSON body and contain an special character we will have the same issue. It doesn't have the highest prio for me but i'll look at it the next few days.

Thanks again for your feedback and the help in investigating the issue.

huserben commented 6 years ago

Hi @jfinnegan3

I updated the tasks (or respectively the service that makes the actual request) to escape characters properly so the JSON sent to the server is valid. With the newest Version (2.5.3) you should be able to remove your special script to escape it yourself and use just the task right away.

In case you test this please let me know if it works or not.

jfinnegan3 commented 6 years ago

I did just that - special script removed and it works perfectly. Thank you again!

huserben commented 6 years ago

Perfect.

Thanks for your feedback and help in solving this issue. Please raise a new one as soon as you find something that is not working as it should of if you have proposals for improvement :-)

jfinnegan3 commented 6 years ago

Hello again @huserben,

I would like to reopen this because I ran into a similar problem where I received the same error.

I now would like to send in the "additional parameters for the build" step a character that needs escaping. I see that we notate every variable with \"...\" to escape the double-quotations, but no other characters that could be contained within the ... are escaped. This would be a good enhancement to your task because I pass in a variable, for example $(Build.SourcesDirectory), which I do not know until queue time.

I could once again introduce the the powershell script, and likely will do that for now, but it would be nice if the task did it itself. Also this would likely change your documentation. Instead of putting those parameters as: \"...\" : \"...\" they'd be written as: "..." : "...", which is neater for users of the task.

This would be a very nice enhancement,

Thanks!

huserben commented 6 years ago

Hi @jfinnegan3

I quickly thought about the parameters after updating the task last time. However it turned out after a quick check that it's not as trivial as initially thought so i left it until needed (which seems to be now :-).

So it definetly makes sense to escape these parameters, but as mentioned it would likely change as well how we define them as we should skip the escaping of the quotes. On the other hand I don't want to break compatibility with existing configurations so I would need to handle the "old style" of the parameters as well.

As I'm a bit busy the next few days this will likely take a week or two to be completly done. In the meantime I hope it's fine to rely on your powershell script.

jfinnegan3 commented 6 years ago

Yes, powershell is working well right now. Thanks for looking into it!

huserben commented 6 years ago

Hi @jfinnegan3

I just uploaded the version 2.6.0 of the task. The build parameters are now escaped as well. On top of this the syntax is simplified so that you can ignore the escaped quotes when defining the parameters. However the old syntax still works to keep compatibility and not cause all the existing builds to fail after an upgrade :-)

Can you let it run for your use case and let me know if it solved your problem?

huserben commented 6 years ago

Hi Jake

just a quick check-up as I did not yet hear from you. Were you able to verify the new functionality so you could remove your script? :-)

chriszuercher commented 6 years ago

hi @huserben I found this issue in GitHub because I was facing a similar issue. I had to use Shrenames as parameters for a triggered build. In the old version I couldn't get it working. After reading about this I tried it again using and with the new version everything works. So at least for my UseCase it is solved.

jfinnegan3 commented 6 years ago

Hello @huserben!

Unfortunately I have not yet put the latest version of the task on my TFS server, and am currently using the PS script still. I'm looking forward to upgrading, but recently busy with other things. Can I suggest one more small enhancement before I do get the latest version?

In my setup, when I'm using the Await Build Task, and the build I'm waiting for fails, I fail the whole build. A couple of the developers on my team have suggested that if that Await Build Task fails as a result of the triggered build failing, could the link to the failed build(s) console be provided in that step? This would allow for easy navigation, so the devs can see what went wrong.

Let me know your thoughts!

huserben commented 6 years ago

Hi @jfinnegan3

ok. However due to the feedback of @chriszuercher I consider this resolved. If you have issues again after updating please reopen the issue. For your improvement, I will take it up, however I "move" it to the issue https://github.com/huserben/TfsExtensions/issues/57 as it includes already a cleanup/change in the logging of awaited builds.

The link to the console you mean just the link to the build that has failed?

jfinnegan3 commented 6 years ago

yes, correct, that is what I mean - a link to the failed build

jfinnegan3 commented 6 years ago

Hello @huserben ,

I recently upgraded to the latest version of the task. I removed the powershell script, and correctly inputted all parameters. Unfortunately, this is not work. As I still received the JSON parsing error. What I'm trying to pass to the triggered build as variable is this:

VariableName: $(Build.SourcesDirectory)

because of the unescaped "\" in the path to the sources directory, the JSON parsing fails.

If you wouldn't mind looking into it again, thank you!

huserben commented 6 years ago

Hmm that is strange, as I tested with the same variable: grafik

Which seems to work properly: grafik

The triggered build itself then as a test just writes the variable to the console to see if it was properly transferred: grafik grafik

Can you tell me which version exactly is in use?

jfinnegan3 commented 6 years ago

You're correct, I made a mistake. I wasn't using the correct version - I thought that I'd updated, but I hadn't. Everything works as intended, sorry for the false alarm!

huserben commented 6 years ago

Well better detect a bug that turns out to not be one than not detecting an actual one ;-)

Thanks for the quick feedback, in this case I'll close the issue. Please don't hesitate to open a new Issue if you find anything else that isn't working or that you would like to have improved.