Validate a Cloud Event on creation and make it read only with a method to augment it

[lholmquist]

• Validate the Cloud Event in the Constructor
• Freeze the Object that is created
• Update/Add extensions after the fact
• Modify other properties if needed
• Update Tests

During the creation of a CloudEvent, the validate method is now called. The object is also frozen to make it read-only.

A new method cloneWith has been added which can be used to add/update attributes such as extensions and will return a new Cloud Event Object

fixes #233

related #29

Signed-off-by: Lucas Holmquist [email protected]

[lholmquist]

I just added Initial support for validating with the Proxy. Some tests are still failing, but I would like to talk about this Proxy handler:

return new Proxy(this, {
      // obj is the instance of the CloudEvent
      // prop is the property being set
      // value is the value on the right side of the equal sign
      set: function (obj, prop: string, value) {
        // Make a copy of the incoming Object, need to have all it's properties and such
        const updateObj = Object.create(obj);
        // Update it with the new value
        updateObj[prop] = value;

        // Validate the object
        obj.validate(updateObj);

        // If we succeed, then Update the real object
        // Set the new value normally
        obj[prop] = value;

        return true;
      },
    });

At first the handler was simpler and looked like this :

set: function (obj, prop, vale) {
  obj[prop] = value;
  return this.validate()
}

edit: i also just realized that we have set methods for data and time that do somthing special, these would most likely need to be moved into the proxy handler

The only problem with this, is if a validation fails, the value passed in was still being assigned. This was causing some tests to fail because we are re-using the same CloudEvent Object across multiple tests. In a real application it might not be an issue, since if an invalid value is assigned, an error is thrown. So not sure if this is overkill and we should re-write the tests or if we should try to validate before assigning. Hopefully what i just rambled out makes sense :)

Also the reason i'm using Object.create and not ... is because ... strips off the set and get methods of time and data and we need those.

[lholmquist]

Going in a different direction. Not using Proxy any more but using the "cloneWith" method instead.

I'll need to update the tests

[lholmquist]

For this PR i had originally thought to add the validation in #228 and #229, but i think those should be its own PR and this one should just focus on calling validate during object creation as well as making the object read-only.

I have another branch with the extension name validation ready to go once this gets in

[lholmquist]

Ping @cloudevents/sdk-javascript-maintainers PTAL

[grant]

Why can't we just add this to the original CloudEvent?

[lholmquist]

probably could. Looks like a leftover of when you couldn't add extensions during construction

[grant]

Same here, can you post the original string, then btoa it?

[lholmquist]

There are other tests that already have the string encoded, so the precedent was already set. If this is a blocker, then we can create another PR to update the other tests as well

[grant]

Maybe add a TODO if other PRs are doing this?
It's still not ideal as I can't read or understand SSB3...

[lance]

LGTM - nice changes to the test files using cloneWith instead of mucking around with a single event, changing it as needed for each test.

[lholmquist]

@cloudevents/sdk-javascript-maintainers how do we feel about merging this?

[lance]

Overall LGTM. I've noted a few places where the tests got looser (just testing expect.to.throw() instead of the type of Error and message). I think these should retain the original intent.These and the others I didn't call out should probably be changed to the stricter throw(ValidationError, "invalid payload").

[lholmquist]

@lance i updated the tests to make sure they are throwing a TypeError, which is the error that will be thrown when trying to delete a property on a frozen object.

[lholmquist]

@grant are you ok with this PR. Your comments were related to how some of the tests were written, which i've tried to give an explanation for. If this is still a blocker, maybe we could address those in a separate PR?

[grant]

Looks good to me. Thanks for the PR!