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

String field for timestamp has incorrect formatting in this client library #2040

Closed
msaniscalchi opened this issue Feb 4, 2022 · 9 comments
Assignees
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@msaniscalchi
Copy link

I'm developing samples for several client libraries. For this library, I see that a string field containing a formatted timestamp is instead returned as an object. Additionally, attempting to print its contents returns a formatted date and time that does not match what is sent over the wire. For example, the Python client library correctly returns "2021-08-18T16:26:05.695663Z" in RFC3339 UTC format, whereas this library returns "8/18/2021 4:26:05 PM". It looks like this library is modifying the formatting of the string timestamp, but we want to use the original formatting.

Environment details

  • Programming language: C#
  • OS: Ubuntu
  • Language runtime version: netcoreapp5.0;

Steps to reproduce

  1.  object createTime = connection.CreateTime;
     Console.WriteLine("\t- Create time: {0}", createTime.ToString());
    

The above should print the string as it is returned in the response, but it instead has some other human-readable formatting applied.

Feel free to contact me internally for additional details.

@msaniscalchi msaniscalchi added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Feb 4, 2022
@amanda-tarafa amanda-tarafa added type: question Request for information or clarification. Not an issue. and removed type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Feb 5, 2022
@amanda-tarafa amanda-tarafa self-assigned this Feb 5, 2022
@amanda-tarafa
Copy link
Contributor

It's not clear to me which library you are using as there are over a hundred in this repo.

In any case chances are that the CreateTime property of the connection has been defined by the API as a datetime or timestamp or similar type and not as a string, in which case the library will correctly expose it as a System.DateTime. You can print a DateTime in whatever format you want, see here https://docs.microsoft.com/en-us/dotnet/api/system.datetime.tostring.

@amanda-tarafa amanda-tarafa added the needs more info This issue needs more information from the customer to proceed. label Feb 6, 2022
@jskeet
Copy link
Collaborator

jskeet commented Feb 7, 2022

In addition to this, date/time values with a date-time type in the Discovery doc are exposed via two properties - one the parsed value as a DateTime, and the other in the raw string format, using a Raw suffix. For example, in Google.Apis.Admin.Reports.reports_v1.cs:

            [Newtonsoft.Json.JsonPropertyAttribute("time")]
            public virtual string TimeRaw { get; set; }

            [Newtonsoft.Json.JsonIgnoreAttribute]
            public virtual System.Nullable<System.DateTime> Time
            {
                get => Google.Apis.Util.Utilities.GetDateTimeFromString(TimeRaw);
                set => TimeRaw = Google.Apis.Util.Utilities.GetStringFromDateTime(value);
            }

Date/time values with a type of google-datetime in the Discovery doc are unfortunately only represented as object properties. I expect those values could be cast to DateTime though (when non-null) as I expect Json.NET is automatically parsing timestamp values.

@msaniscalchi
Copy link
Author

Thanks, I see in the discovery document this field has the type of google-datetime, so that seems consistent with your description of the behavior. The behavior for date-time seems more along the lines of what we would want, as returning an object without any context that it is or should be cast to a DateTime is not very intuitive. Is that behavior for google-datetime intentional?

In the meantime, I'll see if it's possible for us to adjust to date-time.

@jskeet
Copy link
Collaborator

jskeet commented Feb 7, 2022

Is that behavior for google-datetime intentional?

It's far from ideal, but we can't change it now. What we can do is:

  • Add a ...Raw property in the same way that we have for date-time
  • Potentially add a ...DateTime property of type DateTime? to make things simpler

We'd need to think about how these related properties interact with each other, but fundamentally we can add new properties without breaking anything; we can't change existing properties.

@msaniscalchi
Copy link
Author

Both of those options sound like worthwhile improvements and dodge the issue of introducing a breaking change, +1.

At the very least, the first option would resolve this specific issue if there were some reason we couldn't use date-time.

On that note–and we can continue this line of discussion internally–we're not quite sure how to influence the format such that it would change in the discovery document from google-datetime to date-time. If you could point out what would typically need to be modified to make that adjustment, that could help us determine if it's a viable option.

@jskeet
Copy link
Collaborator

jskeet commented Feb 7, 2022

Will start an internal discussion tomorrow.

@amanda-tarafa amanda-tarafa removed the needs more info This issue needs more information from the customer to proceed. label Feb 9, 2022
@jskeet
Copy link
Collaborator

jskeet commented Mar 14, 2022

Just as a status update: we do have a plan around this now, but it's relatively low priority.

@jskeet jskeet added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. priority: p3 Desirable enhancement or fix. May not be included in next release. and removed type: question Request for information or clarification. Not an issue. labels Mar 14, 2022
@msaniscalchi
Copy link
Author

Understood, that makes sense. Thanks for the update.

jskeet added a commit to jskeet/google-api-dotnet-client that referenced this issue Jun 26, 2023
This will implement the changes for googleapis#2424 and googleapis#2040.
jskeet added a commit that referenced this issue Jun 26, 2023
This will implement the changes for #2424 and #2040.
@jskeet
Copy link
Collaborator

jskeet commented Jun 26, 2023

This didn't end up in generator 1.4.18, but is in 1.4.19. Due to release process details, this means the new properties (with Raw and DateTimeOffset suffixes) will only appear as APIs are changed in other ways, or when we do the next support library release. I expect this to be sufficient though, so I'll close this issue now.

@jskeet jskeet closed this as completed Jun 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p3 Desirable enhancement or fix. May not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

3 participants