core.GetProject api fails with error when trying to unmarshal response from azdo

[rguthriemsft]

We have been trying to use the GetProject api passing in project name and getting an error that looks like it can't parse the lastUpdatedTime value in the response.

function that has the bug:

azure-devops-go-api/azuredevops/core/client.go

Line 330 in a196ec5

func (client *Client) GetProject(ctx context.Context, args GetProjectArgs) (*TeamProject, error) {

Error and struct returned:

parsing time ""0001-01-01T00:00:00"" as ""2006-01-02T15:04:05Z07:00"": cannot parse """ as "Z07:00"

&{ 0xc0002bae40 a790779a-4096-43b6-929f-1f1e47917f18 0001-01-01 00:00:00 +0000 UTC 0xc0002bae30 0xc0003305a0 0xc0002bae60 0xc0002bae50 0xc0002baf80 map[collection:map[href:https://dev.azure.com/csedevops/_apis/projectCollections/85fb3d5a-9f21-420f-8de3-fc80bf29054b] self:map[href:https://dev.azure.com/csedevops/_apis/projects/a790779a-4096-43b6-929f-1f1e47917f18] web:map[href:https://dev.azure.com/csedevops/tf_test]] 0xc00008e0e0 0xc0001d8380}

Example code that reproduces the error:

func getProject(ctx context.Context, clients *AggregatedClient) {
	t := true
	f := false
	p := "tf_test"

	args := core.GetProjectArgs{
		ProjectId:           &p,
		IncludeCapabilities: &t,
		IncludeHistory:      &f,
	}

	fmt.Println(args)

	project, err := clients.CoreClient.GetProject(ctx, args)

	if err != nil {
		fmt.Println(err)
	}

	fmt.Println(project)
}

[rguthriemsft]

Looks similar to this issue golang/go#9346

[tedchamb]

Thanks for reporting. I will look into it.

[tedchamb]

Looking for feedback on this issue. I am new to Go, so hoping there is a better way to fix this issue.

To fix the issue, I created a custom struct that has a time.Time property:

type Time struct {
	Time time.Time
}

This allows the creation of a custom converter that checks for the error (we can't change the server behavior, so we have to adapt):

func (t *Time) UnmarshalJSON(b []byte) error {
	t2 := time.Time{}
	err := json.Unmarshal(b, &t2)

	// ignore errors for 0001-01-01T00:00:00 dates. The Azure DevOps service
	// returns default dates in a format that is invalid for a time.Time. The
	// correct value would have a 'z' at the end to represent utc. We are going
	// to ignore this error, and set the value to the default time.Time value.
	// https://github.com/microsoft/azure-devops-go-api/issues/17
	if err != nil {
		if parseError, ok := err.(*time.ParseError); ok && parseError.Value == "\"0001-01-01T00:00:00\"" {
			err = nil
		}
	}

	t.Time = t2
	return err
}

The benefit is that this is an easy fix, and we can handle other nuances with the time.Time type. The drawbacks is that it makes any code dealing with the time more verbose. This is also a breaking change, and therefore it is not a good pattern for other cases for other types in the future, when we have a released version.

Other options considered were to:

1. Generate a custom converter for each struct that has a time.Time field, which handles each field in that struct individually. This would bloat the code greatly, but would work as we want it to.
2. Generate a custom converter for each struct that has a time.Time field, but have that converter call a general func, that reflects into the struct to do the unmarshalling. This is a lot more powerful as it can be adapted to handle all other future deserialization issues without having to make a breaking change, and it doesn't bloat the code. The drawback is in its complexity. At least it seamed complex in Go as I started to dig into it.
3. Check for the error after unmarshalling, and then set the error to nil, if it is this specific error. This is not really an option as this would hide other errors that happen after the initial specific error.

[nmiodice]

@tedchamb can we push on the AzDO team to fix their API response in the default case?

Short of that, I think that your approach makes sense in the short term. The long term IMO is to fix the service as this will impact all the clients.

[tedchamb]

Thanks, @nmiodice. I will definitely push for a fix on the server side, but the 5.1 api version is already released, and can't be changed. We also need to handle talking to already released Azure DevOps Servers.

[rguthriemsft]

@tedchamb I am ok with current approach. We can handle that in the work we are doing for now.

[tedchamb]

@nmiodice , @rguthriemsft , thanks for the feedback! I have pushed the PR.

[tedchamb]

I looked into the code on our server side, and it turns out that this is the default deserialization format from newtonsoft for a default DateTime. You can repo this behavior with a simple program:

using System;
using Newtonsoft.Json;

namespace ConsoleApp24
{
    class Program
    {
        static void Main(string[] args)
        {
            Test t = new Test();
            string serialized = JsonConvert.SerializeObject(t);
            Console.WriteLine(serialized);
        }
    }

    class Test
    {
        public DateTime LastUpdateTime { get; set; }
    }
}

The console output from this is as follows:

{"LastUpdateTime":"0001-01-01T00:00:00"}

This appears to be a bug in newtonsoft if I am reading the spec correctly. The rfc3339 spec shows that the time-offset is required and time-offset = "Z" / time-numoffset.

After doing a quick search through newtonsoft code, it appears that min and max DateTime values are special cased intentionally.