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

resolves #64 by checking the type of ctx.outputData #65

Merged
merged 6 commits into from Jul 5, 2019

Conversation

Projects
None yet
2 participants
@aaslamin
Copy link
Contributor

commented Jul 4, 2019

Issue: #64

aaslamin added some commits Jul 4, 2019

resolves: #64 - add unit tests to ensure panics do not occur if ctx.o…
…utputData does not satisfy elemental.Identifiable

@aaslamin aaslamin requested review from primalmotion and t00f Jul 4, 2019

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "",
    "component": "bahamut",
    "pr-id": "65",
    "commit-sha": "96aea1e1d8575701d0fba40090851b6c71be47d3"
  }
]
evt := elemental.NewEvent(elemental.EventCreate, ctx.outputData.(elemental.Identifiable))
pusher(evt)
switch o := ctx.outputData.(type) {
case elemental.Identifiable:

This comment has been minimized.

Copy link
@primalmotion

primalmotion Jul 4, 2019

Member

What happens if I have a nil identifiable? Can you also add a test case if not already there?

This comment has been minimized.

Copy link
@aaslamin

aaslamin Jul 4, 2019

Author Contributor

Would still work fine due to the fact that nil would be a valid value that an interface can hold - https://golang.org/ref/spec#Switch_statements:

Instead of a type, a case may use the predeclared identifier nil; that case is selected when the expression in the TypeSwitchGuard is a nil interface value. There may be at most one nil case.

See https://play.golang.org/p/d4Atwl3Lysw

package main

import (
	"fmt"
	"encoding/json"
)

func main() {
	switcher("hello") // prints "I am a string!"
	switcher(12) // prints "I am an int!"
	switcher(nil) // prints "I am nil!" 
	switcher(json.RawMessage("amir is awesome!")) // prints "I am something else!"
}

func switcher(i interface{}) {
	switch i.(type) {
	case string:
		fmt.Println("I am a string!")
	case int:
		fmt.Println("I am an int!")
	case nil:
		fmt.Println("I am nil!")
	default:
		fmt.Println("I am something else!")
	}
}

This comment has been minimized.

Copy link
@primalmotion

primalmotion Jul 4, 2019

Member

The point is we don't want to perform a push if nil

This comment has been minimized.

Copy link
@aaslamin

aaslamin Jul 4, 2019

Author Contributor

That seems like something that the eventPusherFunc should be responsible for checking then?

This comment has been minimized.

Copy link
@aaslamin

aaslamin Jul 4, 2019

Author Contributor

Correct me if I am wrong, but the only way to pass in a nil Identifiable would be if you have a pointer to a type that satisfies that interface and the value you assign to the pointer is nil. So in that situation, you are clearly doing something strange and it should 💥

Right?

This comment has been minimized.

Copy link
@aaslamin

aaslamin Jul 5, 2019

Author Contributor

And this will NOT cause a panic because you will never be able to pass in a nil interface if you follow the call path from here:

  • elemental.NewEvent which will be passed a non-nil interface value, which calls
    • NewEventWithEncoding which will call Encode with a non-nil interface value
      • Encode will run, which will not trigger this condition as the interface value passed in is not nil

This comment has been minimized.

Copy link
@aaslamin

aaslamin Jul 5, 2019

Author Contributor

To summarize the last few comments: passing in a nil Identifiable will not cause a panic.

This comment has been minimized.

Copy link
@primalmotion

primalmotion Jul 5, 2019

Member

Add a test about that then :)

This comment has been minimized.

Copy link
@aaslamin

aaslamin Jul 5, 2019

Author Contributor

Will do 😉

This comment has been minimized.

Copy link
@aaslamin

aaslamin Jul 5, 2019

Author Contributor

Done!

@codecov

This comment has been minimized.

Copy link

commented Jul 4, 2019

Codecov Report

Merging #65 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #65   +/-   ##
=======================================
  Coverage   88.24%   88.24%           
=======================================
  Files          28       28           
  Lines        2195     2195           
=======================================
  Hits         1937     1937           
  Misses        231      231           
  Partials       27       27
Impacted Files Coverage Δ
dispatchers.go 100% <100%> (ø) ⬆️

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 17f3023...3ae5ed0. Read the comment docs.

@aaslamin aaslamin requested a review from primalmotion Jul 4, 2019

aaslamin added some commits Jul 5, 2019

new: add new test to verify that no panic occurs if output data conta…
…ins a nil elemental.Identifiable - dispatchCreateOperation
new: add new test to verify that no panic occurs if output data conta…
…ins a nil elemental.Identifiable - dispatchUpdateOperation
new: add new test to verify that no panic occurs if output data conta…
…ins a nil elemental.Identifiable - dispatchDeleteOperation
new: add new test to verify that no panic occurs if output data conta…
…ins a nil elemental.Identifiable - dispatchPatchOperation
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "",
    "component": "bahamut",
    "pr-id": "65",
    "commit-sha": "3ae5ed0adb6073713240383dae7e993e4a49219a"
  }
]
@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

Update

  • Added test coverage to verify existing behaviour that a panic will not occur if caller provides a nil elemental.Identifiable - check last 4 commits.

👀 ready for potentially a final review 🙏 @primalmotion

@primalmotion
Copy link
Member

left a comment

LGTM. run FT before merging

@aaslamin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "",
    "component": "bahamut",
    "pr-id": "65",
    "commit-sha": "3ae5ed0adb6073713240383dae7e993e4a49219a"
  },
  {
    "project": "",
    "component": "backend",
    "pr-id": "452",
    "commit-sha": "ffaa97046a01b260e543ae32d417513addb3c2aa"
  }
]

@aaslamin aaslamin changed the title Resolves #64 by checking the type of ctx.outputData resolves #64 by checking the type of ctx.outputData Jul 5, 2019

@aaslamin aaslamin merged commit 967d91c into master Jul 5, 2019

6 checks passed

built
Details
codecov/patch 100% of diff hit (target 88.24%)
Details
codecov/project 88.24% (+0%) compared to 17f3023
Details
functional-tests Submitter: reason: . functional-tests set to success
Details
functional-tests-trigger Submitter: reason: . functional-tests-trigger set to success
Details
unit-tests
Details

@aaslamin aaslamin deleted the fixes-issue-64 branch Jul 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.