fix: do not modify incoming event's specversion #419
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Even if the specversion is totally invalid, we should not change the value received in an incoming `Message`. Previously we defaulted to 1.0 if we did not recognize the version number. This commit changes that, leaving the value unmodified. We default to parsing this mystery event with the 1.0 spec. When the event is validated with `event.validate()` we return `false`. One additional small change to eliminate a prettier warning about `parer` being previously declared. Fixes: cloudevents#332 Fixes: cloudevents#333 Signed-off-by: Lance Ball <[email protected]>
lance
added
type/fix
lholmquist
approved these changes
May 25, 2021
lholmquist
pushed a commit
to lholmquist/sdk-javascript
that referenced
this pull request
Jul 6, 2021
Even if the specversion is totally invalid, we should not change the value received in an incoming `Message`. Previously we defaulted to 1.0 if we did not recognize the version number. This commit changes that, leaving the value unmodified. We default to parsing this mystery event with the 1.0 spec. When the event is validated with `event.validate()` we return `false`. One additional small change to eliminate a prettier warning about `parer` being previously declared. Fixes: cloudevents#332 Fixes: cloudevents#333 Signed-off-by: Lance Ball <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Even if the specversion is totally invalid, we should not change the valuereceived in an incoming
Message. Previously we defaulted to 1.0 if we did not recognize the version number. This commit changes that, leaving the value unmodified. We default to parsing this mystery event with the 1.0 spec. When the event is validated withevent.validate()we returnfalse.One additional small change to eliminate a prettier warning about
pasrerbeing previously declared.As discussed in #332 this same problem exists for both
idandtime. However, I am not addressing these at the moment. I think we should simply reconsider ANY defaults. Regardingidandtime, the behavior is different with each. Withidmissing, it is not possible to parse an incomingMessage, since we use the ID to determine if it's a structured event. With noidpresent, it's just not an event. The spec states that you determine whether an incoming HTTP request is a structured cloud event by checking force-idin the headers. So... 🤷Regarding
time, it's not currently possible to construct an event that does NOT have atimeproperty. When using thenew CloudEvent()constructor, a default value is created if one isn't provided. However,eventobjects are read only, so settingevent.time = undefinedafter the fact doesn't work. If instead, we accept the incoming message, create an event, check to see if the original message had atimeattribute, and if not useevent.cloneWith({ time: undefined })- we're right back where we started, sincecloneWith()uses the constructor internally.Given that, I think the only answer is for us to remove the default value generated for
time.Fixes: #332
Fixes: #333
Signed-off-by: Lance Ball [email protected]