Add UpdateSubscription endpoint #398

alexdebrie · 2018-03-23T22:04:35Z

What did you implement:

Adds an UpdateSubscription API endpoint in the Config API. For now, this should only be called if updating the CORS config on an HTTP subscription as the rest of the metadata is included in the subscriptionId.

How did you implement it:

Most of the implementation is pretty straight-forward. As noted above, we should only allow a Subscription to be updated ~~if the CORS config is being changed, as the rest of the Subscription data is included in the subscriptionID.~~ in very limited scenarios. If you're changing one of those elements, you're not updating a Subscription but rather creating a new Subscription.

To handle this check, I generate a subscriptionID and compare it against the existing subscriptionID. If they're different, I throw an error. I thought this would be better than explicitly checking for CORS changes or otherwise comparing the update to the existing. It also should work in the future if we add additional metadata to the Subscription object that's not in the subscription ID. make sure certain values haven't changed and throw an error if they do.

How can we verify it:

Start the Event Gateway locally, then:

Create a function:

curl -X POST \
  http://localhost:4001/v1/spaces/default/functions \
  -H 'Content-Type: application/json' \
  -d '{
	"functionId": "awslambda",
	"type": "awslambda",
	"provider": {
		"arn": "test",
		"region": "us-west-2",
		"awsAccessKeyId": "test",
		"awsSecretAccessKey": "secret"
	}
}'

Create a subscription:

curl -X POST \
  http://localhost:4001/v1/spaces/default/subscriptions \
  -H 'Content-Type: application/json' \
  -d '{
	"functionId": "awslambda", 
	"event": "http",
	"method": "GET",
	"path": "/"
}'

Update the subscription by adding CORS:

curl -X PUT \
  'http://localhost:4001/v1/spaces/default/subscriptions/http,GET,%2F' \
  -H 'Content-Type: application/json' \
  -d '{
	"functionId": "awslambda", 
	"event": "http",
	"method": "GET",
	"path": "/",
	"cors": {
		"origins": ["*"]
	}
}'

This should succeed and return the full subscription:

{"space":"default","subscriptionId":"http,GET,%2F","event":"http","functionId":"awslambda","method":"GET","path":"/","cors":{"origins":["*"],"methods":["HEAD","GET","POST"],"headers":["Origin","Accept","Content-Type"],"allowCredentials":false}}

Attempt to update the subscription by changing the method to POST:

curl -X PUT \
  'http://localhost:4001/v1/spaces/default/subscriptions/http,GET,%2F' \
  -H 'Content-Type: application/json' \
  -d '{
	"functionId": "awslambda", 
	"event": "http",
	"method": "POST",
	"path": "/",
	"cors": {
		"origins": ["*"]
	}
}'

This should fail as it would change the subscription ID:

{"errors":[{"message":"Invalid update. 'Method' of existing subscription cannot be updated."}]}

Todos:

Write tests
Write documentation
Fix linting errors
Make sure code coverage hasn't dropped
Provide verification commands / resources
Enable "Allow edits from maintainers" for this PR
Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

codecov · 2018-03-24T02:36:19Z

Codecov Report

Merging #398 into master will increase coverage by 1.57%.
The diff coverage is 85.5%.

@@            Coverage Diff             @@
##           master     #398      +/-   ##
==========================================
+ Coverage   61.14%   62.72%   +1.57%     
==========================================
  Files          26       26              
  Lines        1521     1588      +67     
==========================================
+ Hits          930      996      +66     
+ Misses        554      550       -4     
- Partials       37       42       +5

Impacted Files	Coverage Δ
httpapi/httpapi.go	`54.28% <100%> (+10.15%)`	⬆️
libkv/subscription.go	`87.62% <73.68%> (-0.35%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f673e72...a678883. Read the comment docs.

mthenw · 2018-03-27T13:54:34Z

libkv/subscription.go

+		return nil, err
+	}
+
+    // If the subscriptionID changes, it should be a new subscription rather than an update.


I think it should be approached differently. validateSubscription should check if event, functionId, path and method are the same, if not it should return validation error. If those values are the same we can just update the key in etcd.

This approach causes that in some cases if we will change the subscription ID we will also need to change this logic. There is no need to do that.

This approach causes that in some cases if we will change the subscription ID we will also need to change this logic

Wouldn't we need a migration of existing subscriptions anyway in that case? Also, if we do change the how the subscriptionID is calculated, wouldn't that be implemented in the generateSubscriptionID method?

It would, but we should not treat ID as something that contains meaningful ingo. It can be easily achieved by comparing current values with new values, it's just safer and more obvious. It's a bit more confusing the IDs are compared..

mthenw · 2018-03-27T13:55:23Z

subscription/errors.go

+}
+
+func (e ErrInvalidSubscriptionUpdate) Error() string {
+	return fmt.Sprintf("Invalid update. This update would change the SubscriptionID for %q.", e.ID)


I'm not sure if that's important to user. The more important would be "Path of existing subscription cannot be update." or something.

mthenw

LGTM, two small requests though.

mthenw · 2018-03-28T14:28:28Z

README.md

+**Request**
+
+* `event` - `string` - event name
+* `functionId` - `string` - ID of function to receive events


can you add some info that those fields cannot be updated?

mthenw · 2018-03-28T14:53:57Z

libkv/subscription.go

@@ -65,6 +65,48 @@ func (service Service) CreateSubscription(s *subscription.Subscription) (*subscr
 	return s, nil
 }

+// UpdateSubscription updates subscription.
+func (service Service) UpdateSubscription(id subscription.ID, s *subscription.Subscription) (*subscription.Subscription, error) {
+	err := service.validateSubscription(s)


I know that you didn't introduce that but can you remove validateSubscription method from the struct and just make it regular function like validateSubscriptionUpdate?

mthenw · 2018-03-28T20:54:01Z

README.md

+
+Status code:
+
+* `200 Created` on success


404 if subscription not found

mthenw · 2018-03-28T20:54:11Z

docs/openapi/openapi-config-api.yaml

+            application/json:
+              schema:
+                $ref: "#/components/schemas/Subscription"
+        400:


404 missing

mthenw · 2018-03-28T20:55:06Z

httpapi/httpapi.go

+		if _, ok := err.(*subscription.ErrInvalidSubscriptionUpdate); ok {
+			w.WriteHeader(http.StatusBadRequest)
+		} else if _, ok := err.(*subscription.ErrSubscriptionNotFound); ok {
+			w.WriteHeader(http.StatusBadRequest)


it should be 404

mthenw · 2018-03-28T20:55:39Z

httpapi/httpapi_test.go

+
+	httpresp := &httpapi.Response{}
+	json.Unmarshal(resp.Body.Bytes(), httpresp)
+	assert.Equal(t, http.StatusBadRequest, resp.Code)


Add UpdateSubscription endpoint

5244dd8

alexdebrie requested a review from mthenw March 23, 2018 22:04

alexdebrie added 2 commits March 24, 2018 00:52

Add missing mock

2cfe0cd

Add tests

510b352

alexdebrie added 4 commits March 24, 2018 02:37

Merge branch 'master' into UpdateSubscription

f36333b

Update docs and OpenAPI

468730c

Add httpapi tests

0cbd9dc

Merge branch 'master' into UpdateSubscription

059cc1a

mthenw suggested changes Mar 27, 2018

View reviewed changes

Fix how we check for valid updates

1836901

mthenw suggested changes Mar 28, 2018

View reviewed changes

Update README and change method to function

43ffdbc

mthenw approved these changes Mar 28, 2018

View reviewed changes

Change for 404 if subscription not found

a678883

mthenw merged commit acfe7bd into master Mar 29, 2018

mthenw deleted the UpdateSubscription branch March 29, 2018 06:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UpdateSubscription endpoint #398

Add UpdateSubscription endpoint #398

alexdebrie commented Mar 23, 2018 •

edited

Loading

codecov bot commented Mar 24, 2018 •

edited

Loading

mthenw Mar 27, 2018

alexdebrie Mar 27, 2018

mthenw Mar 28, 2018

mthenw Mar 27, 2018

mthenw left a comment

mthenw Mar 28, 2018

mthenw Mar 28, 2018

mthenw Mar 28, 2018

mthenw Mar 28, 2018

mthenw Mar 28, 2018

mthenw Mar 28, 2018

Add UpdateSubscription endpoint #398

Add UpdateSubscription endpoint #398

Conversation

alexdebrie commented Mar 23, 2018 • edited Loading

What did you implement:

How did you implement it:

How can we verify it:

Todos:

codecov bot commented Mar 24, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mthenw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexdebrie commented Mar 23, 2018 •

edited

Loading

codecov bot commented Mar 24, 2018 •

edited

Loading