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

Implement wrapper for google.protobuf.Timestamp, and correctly generate wrappers for static target. #1258

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ntindall
Copy link

@ntindall ntindall commented Jun 26, 2019

@dcodeIO @alexander-fenster @JustinBeckwith can you help me understand what else is needed to address this?

Related: #351 #893 #916 #1108 #1304

@ntindall ntindall force-pushed the nt-google.protobuf.Timestamp branch from b390fe7 to 8931805 Compare June 26, 2019 01:49
@ntindall ntindall changed the title Implement wrapper for google.protobuf.Timestamp Implement wrapper for google.protobuf.Timestamp, and correctly generate wrappers for static target. Jun 26, 2019
@ntindall
Copy link
Author

ntindall commented Jul 9, 2019

any update here? I'd like to get this in to the mainline if we can.

@ntindall
Copy link
Author

@alexander-fenster @JustinBeckwith @dcodeIO any feedback on this would be appreciated.

@ntindall
Copy link
Author

ntindall commented Oct 6, 2019

@nicolasnoble any chance you can take a look here?

@kheyse-werk
Copy link

kheyse-werk commented Oct 22, 2019

Maybe I do something wrong, but it seems that "fromObject" does not work with iso time strings.
So you can't to type.toObject(type.fromObject({myFieldTime: "1970-01-01T00:00:05.000Z"})).
I get the error: "object expected"

The simplest solution I could find is to replace

("if(typeof d%s!==\"object\")", prop)

with

("if(false)") 

But I have no idea what this may break.

@ntindall
Copy link
Author

@kheyse-oqton could you provide a more complete example of the error scenario you are hitting? not sure I fully understand.

@kheyse-werk
Copy link

@ntindall
Copy link
Author

Thanks @kheyse-oqton - thats helpful. I'll look into that.

@gfecher
Copy link

gfecher commented Jan 15, 2020

Any update on this? I would love to have this functionality.

@untra
Copy link

untra commented Feb 11, 2020

Serious +1 for this PR. Native support for google.protobuf.Timestamp fields would significantly help, considering the prevalence of this proto type and the number of +1s and associated issues related to this PR.

@guyisra
Copy link

guyisra commented Mar 19, 2020

any updates on this?

@untra
Copy link

untra commented Mar 19, 2020

we just bit the bullet replaced all of our google.protobuf.Timestamp fields with int64s. This worked for us because we have a relatively new codebase. I would advise new projects to do similar.

Copy link
Contributor

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. Would be nice to get that in.

Could you pull in the test case from #1258 (comment)?

Even if we don't hear from maintainers, this can be added by the caller of the library as follows (random playground code, TypeScript):

import * as proto from "protobufjs";

// ...

const root = proto
  .parse(source)
  .root.addJSON(proto.common.get("google/protobuf/timestamp.proto")!.nested!)
  .addJSON(proto.common.get("google/protobuf/any.proto")!.nested!)
  .resolveAll();
// const root = parsed.root;

// Obtain a message type
const AwesomeMessage = root.lookupType("awesomepackage.AwesomeMessage");

proto.wrappers[".google.protobuf.Timestamp"] = {
  fromObject: function (object) {
    return this.fromObject(object);
  },

  toObject: function (message, options) {
    console.log(message);

    if (!(message instanceof this.ctor) && message instanceof proto.Message) {
      const object = message.$type.toObject(message, options);
      object["@blub"] = "extra stuff";
      return object;
    }

    // (message as any).blub = 1233;
    return this.toObject(message, options);
  },
};

options = {};

// decode value if requested
if (options && options.json) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options is always truthy here as it is normalized above


// decode value if requested
if (options && options.json) {
return new Date(message.seconds*1000 + message.nanos/1000000).toISOString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be very cool to comply with the 0, 3, 6 or 9 fractional digits rule from https://developers.google.com/protocol-buffers/docs/proto3#json

Example implementation: https://github.com/gogo/protobuf/blob/v1.3.1/jsonpb/jsonpb.go#L250-L266

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think this needs to be fixed before this can be merged. We did this on our internal patch.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return new Date(message.seconds*1000 + message.nanos/1000000).toISOString();
// golang JSON serialization truncates extra points of precision.
// we do the same.
// https://github.com/golang/protobuf/blob/master/jsonpb/encode.go#L197
var isoDateString = new Date(message.seconds*1000 + message.nanos/1000000).toISOString();
isoDateString = isoDateString.replace(/[.]000Z$/, 'Z');
return isoDateString;

(needs to be updated to handle 6 and 9 points of precision).

return this.fromObject(object);
}
//Convert ISO-8601 to epoch millis
var dt = Date.parse(object);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation cuts off information, since native Date can only store 3 fractional digits:

> new Date(Date.parse("1970-01-01T00:00:05.111822333Z")).toISOString()
'1970-01-01T00:00:05.111Z'

It is not too hard to implement the parser manually, allowing to keep the precision.

@chunshengji
Copy link

this looks great, can we get this one updated and merged?

@bangbang93
Copy link

+1 keep watching

@Luabee
Copy link

Luabee commented Dec 10, 2020

Still hoping this gets merged

@kivancguckiran
Copy link

Any update on the PR?

@john-michael-murphy
Copy link

Such a great feature! Any chance we can get this merged?

@ntindall
Copy link
Author

I'm happy to help finish this / once there is better support from the maintainers. @alexander-fenster, maybe we can coordinate, I see you have a related PR out.

@alexander-fenster
Copy link
Contributor

alexander-fenster commented Aug 4, 2021

(copying my note from #1495 which was an attempt to add wrappers for several other types)


Folks, for those who follows this PR and, in general, follows the topic of proper proto3 JSON serialization/deserialization according to the protobuf spec, I wanted to post a quick update here. For reasons not directly related to anything in this PR, I won't be able to complete it here soon, so I released the proto3 JSON serialization/deserialization code as a separate npm package.

$ npm install proto3-json-serializer

At this moment, we only support instances of protobuf.Message which should be enough for most use cases. Feel free to try it and create issues in that repository in case if you need anything to be fixed; contributions are also welcome :)

This separate package will cover the workflows I need at this time; if I have some free time to backport its functionality directly to protobuf.js (e.g. by generating .fromProto3JSON / .toProto3JSON together with .fromObject / .toObject), I'll send a separate pull request here, but I cannot promise any ETA on that.

@ideasculptor
Copy link

ideasculptor commented Oct 28, 2021

Even if we don't hear from maintainers, this can be added by the caller of the library as follows (random playground code, TypeScript):

import * as proto from "protobufjs";

// ...

const root = proto
  .parse(source)
  .root.addJSON(proto.common.get("google/protobuf/timestamp.proto")!.nested!)
  .addJSON(proto.common.get("google/protobuf/any.proto")!.nested!)
  .resolveAll();
// const root = parsed.root;

// Obtain a message type
const AwesomeMessage = root.lookupType("awesomepackage.AwesomeMessage");

proto.wrappers[".google.protobuf.Timestamp"] = {
  fromObject: function (object) {
    return this.fromObject(object);
  },

  toObject: function (message, options) {
    console.log(message);

    if (!(message instanceof this.ctor) && message instanceof proto.Message) {
      const object = message.$type.toObject(message, options);
      object["@blub"] = "extra stuff";
      return object;
    }

    // (message as any).blub = 1233;
    return this.toObject(message, options);
  },
};

Just reinforcing the point, for those that missed the earlier message, that you can relatively easily get fromObject() and toObject() to convert Date instances to Timestamp protobufs by adding your own wrapper - a very convenient mechanism for converting from any arbitrary object to a particular protobuf that I don't believe is addressed in the docs anywhere.

My code looks like this, and always uses Date instances when converting between javascript objects and protos. Unfortunately, I don't think there is a convenient way to set up the same conversion when calling create() with a plain javascript object. If there is, I'd love to hear about it. Perhaps I can add a create() wrapper to the fromObject and toObject wrappers I already have? That thought just occurred to me.

protobuf.wrappers[".google.protobuf.Timestamp"] = {
  fromObject: function fromObject(object) {
    if (typeof object !== 'string') {
      if (object instanceof Date) {
        return this.fromObject({
          seconds: Math.floor(object/1000),
          nanos: (object % 1000) * 1000000
        });
      }
      return this.fromObject(object)
    }
    var dt = Date.parse(object);
    if (isNaN(dt)) {
      // not a number, default to the parent implementation
      return this.fromObject(object);
    }
    return this.create({
      seconds: Math.floor(dt/1000),
      nanos: (dt % 1000) * 1000000
    });
  },
  toObject: function toObject(message, options) {
    return new Date(message.seconds*1000 + message.nanos/1000000)
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.