Fix for possible exception when fetching work item information #1395

drolevar · 2021-09-28T15:23:40Z

When trying to convert a legacy repo, I was getting the following exception, probably because some internal discrepancies in the project definition:
TF201077: The work item type cannot be found. It may have been renamed or destroyed.
I've added this try/catch to enable the process to continue even if some work item information cannot be fetched.

siprbaum

Overall, I am sympathetic to your cause and only have some small change requests.
Please also add an entry ala
An error in fetching the workitems associated to a changeset no longer aborts the fetch/clone (#1395 @drolevar)
to the release notes here:
https://github.com/git-tfs/git-tfs/blob/master/doc/release-notes/NEXT.md

siprbaum · 2021-09-30T06:07:57Z

src/GitTfs.VsCommon/TfsHelper.Common.cs

+                        Url = Linking.GetArtifactUrl(wi.Uri.AbsoluteUri)
+                    });
+                }
+                catch (TeamFoundationServerException e)


I am currious, do you remember which property caused the exception? At least the code in HasWorkItems method suggests that the WorkItems array can be fetched.

Please check for the concrete error TF201077, to ensure we are not accidentially ignoring other errors.
See e.g. the code in HasWorkItems a few lines below, how to check the exception message method, inlined here

protected virtual bool HasWorkItems(Changeset changeset) { // This method wraps changeset.WorkItems, because // changeset.WorkItems might result to ConnectionException: TF26175: Team Foundation Core Services attribute 'AttachmentServerUrl' not found. // in this case assume that it is initialized to null // NB: in VS2011 a new property appeared (AssociatedWorkItems), which works correctly WorkItem[] result = null; try { result = Retry.Do(() => changeset.WorkItems); } catch (ConnectionException exception) { if (!exception.Message.StartsWith("TF26175:")) throw; } return result != null && result.Length > 0; }

So I've refactored the code a bit using both AssociatedWorkItems (which works even for malformed work items) and WorkItems to update the TfsWorkItem instances (which can cause an exception).

siprbaum

See the commens inline

siprbaum · 2021-09-30T16:36:03Z

src/GitTfs.VsCommon/TfsHelper.Common.cs

-            // in this case assume that it is initialized to null
-            // NB: in VS2011 a new property appeared (AssociatedWorkItems), which works correctly
-            WorkItem[] result = null;
+            var workItems = changeset.AssociatedWorkItems.Select(wi => new TfsWorkitem


Can we have a comment in this method explaining why we are first using the AssociatedWorkItems and then
the WorkItems?
I already have spent several minutes on the Join below to understand that it will only return the WorkItems which are in both sets (the intersection). I fear that in half a year, I would be completely lost when looking at the code again.

siprbaum · 2021-09-30T16:44:36Z

src/GitTfs.VsCommon/TfsHelper.Common.cs

-                if (!exception.Message.StartsWith("TF26175:"))
-                    throw;
+                Trace.TraceWarning(e.Message);
+                return workItems;


Here we return the only half filled TfsWorkitems in case we have an error.
Furthermore, if e.g. AssociatedWorkItems.Lenght == 3 and we fail on the first work item when joining,
then all the subsequent work items will be silently ignored. I believe your first version didn't have this problem.

On the other hand you mentioned that due the prefiltering on the AssociatedWorkItems the error no longer exists.
Perhaps this is an artificial problem only?
Can you double check if this is true? If so, we So if there are e.g.

siprbaum · 2021-09-30T16:57:11Z

src/GitTfs.VsCommon/TfsHelper.Common.cs


-            return result != null && result.Length > 0;
+        protected virtual bool HasWorkItems(Changeset changeset)


This method not longer needs to be overriden in any of its users, so either make it

Suggested change

protected virtual bool HasWorkItems(Changeset changeset)

private bool HasWorkItems(Changeset changeset)

or better remove it altoegether and call GetWorkItems unconditonally and let it return an Enumerable.Empty<ITfsWorkitem>() if there are none.

siprbaum · 2021-09-30T17:25:30Z

Edited to fix a formating error

On another note and while you are looking in this area, I was wondering if we can get rid of the access to WorkItems altoegether and simply use AssociatedWorkItems instead. (Not saying you have to do it, but want to pick your brain here)

We would need to get rid of these two fields

    public interface ITfsWorkitem
    {
        int Id { get; set; }
        string Title { get; set; }
-       string Description { get; set; }
-       string Url { get; set; }
    }

as they have no representation in the AssociatedWorkItems.

Description is currently unused, so can be removed without any harm right now.
Url is used, but I just checked for one of my projects imported in git-tfs and the URL is not usuable.
It has the format like
https://some.server.com/tfs/tpc/WorkItemTracking/WorkItem.aspx?artifactMoniker=<ID>
but when I try to open it in the browser I get the following error:
TF200016: The following project does not exist: WorkItemTracking. Verify that the name of the project is correct and that the project exists on the specified Azure DevOps Server.

@drolevar Are the URLs of the WorkItems usable on your end?

My current working theory is that this URL schema was working in the past, e.g. with an older TFS server the URL
was valid but with more recent TFS versions it no longer works.
And if this is the case and no one complaint so far, we can get rid of it altoegether.

Adding @pmiossec to the loop, as he introduced the URL in b5fb9b5, just in case he has some recollection on that.

drolevar · 2021-09-30T18:52:17Z

@siprbaum I agree with your suggestion of using AssociatedWorkItems instead of WorkItems.

pmiossec · 2021-09-30T19:35:19Z

Adding @pmiossec to the loop, as he introduced the URL in b5fb9b5, just in case he has some recollection on that.

I have added this url because at the time TFS server was displaying something or redirecting it.
Maybe if you are able to debug the code, you could check if it has been replaced by another property that return a useful link for a workitem otherwise, yes, you could remove it because it is no more useful...

siprbaum · 2021-10-05T11:31:57Z

I created #1396 to fix the wrong URLs and also prepare for switching to AssociatedWorkItems by getting rid of
unused properties. We also no longer require the URI from the WorkItem, as the new way to get the URL just requires the ID.

Any change you (@drolevar or @pmiossec) could have a look at it? If we can agree that this is the way forward, we can merge ot #1396 first and then rebase the current PR on top of that, which would make the code simpler.

drolevar · 2021-10-05T13:39:04Z

@siprbaum I agree. Please, merge #1396 and I'll modify my changes accordingly.

siprbaum · 2021-10-07T17:21:49Z

@drolevar I'll merged the preperations in the meantime, so the code can be rebased.

I did only get rid of the Description property, but now the usage of URI doesn't hurt us anymore and we still can get away with
using the AssociatedWorkItems only.

drolevar · 2021-10-13T12:49:39Z

@siprbaum OK, updated the PR. Honestly, it was more removing than adding. Verified that it works both for changesets with and without associated work items.

siprbaum

I looked at the changes and they are reasonable.

One last thing, can you please --amend the commit message to describe what the original problem was and how the fix prevents this error to happen? I prefere to have this in the commit message so that when I later need to look at the code histroy I have the explanation right there.
It is much easier to deal with it that way than to go hunting in the github PR for it.

Otherwise, I think this can go in.

In certain cases of legacy TFS projects using Changeset::WorkItems could cause a WorkItemTypeDeniedOrNotExistException (error TF201077) and cloning process stopped. This change uses AssociatedWorkItems instead which works fine even in this case.

siprbaum · 2021-10-18T10:15:55Z

@drolevar Thanks for your work, I didn't notice that you already changed the message, otherwise I would have merged it earlier.

siprbaum reviewed Sep 30, 2021

View reviewed changes

drolevar force-pushed the fix_tfs_exception branch from cca14a8 to d7e88bb Compare September 30, 2021 13:24

siprbaum requested changes Sep 30, 2021

View reviewed changes

siprbaum mentioned this pull request Oct 5, 2021

Fix incorrect workitem URLs #1396

Merged

siprbaum added the awaiting:contributer Feedback from the contributer is need label Oct 7, 2021

drolevar force-pushed the fix_tfs_exception branch from d7e88bb to a9cb007 Compare October 13, 2021 12:43

siprbaum removed the awaiting:contributer Feedback from the contributer is need label Oct 13, 2021

siprbaum requested changes Oct 13, 2021

View reviewed changes

drolevar force-pushed the fix_tfs_exception branch from a9cb007 to f710a91 Compare October 14, 2021 08:43

siprbaum merged commit d1c18c1 into git-tfs:master Oct 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for possible exception when fetching work item information #1395

Fix for possible exception when fetching work item information #1395

drolevar commented Sep 28, 2021

siprbaum left a comment

siprbaum Sep 30, 2021

drolevar Sep 30, 2021

siprbaum left a comment

siprbaum Sep 30, 2021

siprbaum Sep 30, 2021

siprbaum Sep 30, 2021

siprbaum commented Sep 30, 2021 •

edited

drolevar commented Sep 30, 2021

pmiossec commented Sep 30, 2021

siprbaum commented Oct 5, 2021

drolevar commented Oct 5, 2021

siprbaum commented Oct 7, 2021 •

edited

drolevar commented Oct 13, 2021

siprbaum left a comment

siprbaum commented Oct 18, 2021


		return result != null && result.Length > 0;
		protected virtual bool HasWorkItems(Changeset changeset)

	protected virtual bool HasWorkItems(Changeset changeset)
	private bool HasWorkItems(Changeset changeset)

Fix for possible exception when fetching work item information #1395

Fix for possible exception when fetching work item information #1395

Conversation

drolevar commented Sep 28, 2021

siprbaum left a comment

Choose a reason for hiding this comment

siprbaum Sep 30, 2021

Choose a reason for hiding this comment

drolevar Sep 30, 2021

Choose a reason for hiding this comment

siprbaum left a comment

Choose a reason for hiding this comment

siprbaum Sep 30, 2021

Choose a reason for hiding this comment

siprbaum Sep 30, 2021

Choose a reason for hiding this comment

siprbaum Sep 30, 2021

Choose a reason for hiding this comment

siprbaum commented Sep 30, 2021 • edited

drolevar commented Sep 30, 2021

pmiossec commented Sep 30, 2021

siprbaum commented Oct 5, 2021

drolevar commented Oct 5, 2021

siprbaum commented Oct 7, 2021 • edited

drolevar commented Oct 13, 2021

siprbaum left a comment

Choose a reason for hiding this comment

siprbaum commented Oct 18, 2021

siprbaum commented Sep 30, 2021 •

edited

siprbaum commented Oct 7, 2021 •

edited