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

Add capabilty to inject custom fields in requests #246

Merged
merged 2 commits into from
Jul 12, 2019

Conversation

alexandrebouthinon
Copy link
Member

@alexandrebouthinon alexandrebouthinon commented Jul 11, 2019

What does this PR do?

Add capabilty to inject custom fields in requests. According to #245

How should this be manually tested?

I used this little program to test this new feature:

package main

import (
	"log"
	"os"

	"github.com/kuzzleio/sdk-go/kuzzle"
	"github.com/kuzzleio/sdk-go/protocol/websocket"
	"github.com/kuzzleio/sdk-go/types"
)

func main() {
	// Creates a WebSocket connection.
	// Replace "kuzzle" with
	// your Kuzzle hostname like "localhost"
	c := websocket.NewWebSocket("localhost", nil)
	// Instantiates a Kuzzle client
	kuzzle, _ := kuzzle.NewKuzzle(c, nil)
	// Connects to the server.
	err := kuzzle.Connect()
	if err != nil {
		log.Fatal(err)
		os.Exit(1)
	}
	ch := make(chan *types.KuzzleResponse)
	options := types.NewQueryOptions()

	query := types.KuzzleRequest{}
	query.AddCustomArg("cert", "cert")
	query.AddCustomArg("foo", "bar")

	go kuzzle.Query(&query, options, ch)
	<-ch

	// Disconnects the SDK
	kuzzle.Disconnect()
}

But it's very hard to check the WebSocket message so I used Wireshark:
image

@alexandrebouthinon alexandrebouthinon force-pushed the 245-custom-args-requests branch from b29c0a5 to 465c56f Compare July 11, 2019 16:06
Copy link

@Tommy647 Tommy647 left a comment

Choose a reason for hiding this comment

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

Looks good to me

@codecov
Copy link

codecov bot commented Jul 11, 2019

Codecov Report

Merging #246 into 2-dev will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##            2-dev     #246      +/-   ##
==========================================
+ Coverage   88.86%   88.91%   +0.05%     
==========================================
  Files         244      244              
  Lines        4463     4466       +3     
==========================================
+ Hits         3966     3971       +5     
+ Misses        443      441       -2     
  Partials       54       54
Impacted Files Coverage Δ
kuzzle/query.go 59.45% <100%> (+1.71%) ⬆️
realtime/subscribe.go 76.47% <0%> (+5.88%) ⬆️

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 31ac1ec...465c56f. Read the comment docs.

@alexandrebouthinon alexandrebouthinon changed the base branch from 1-dev to 2-dev July 11, 2019 16:27
Copy link
Contributor

@Aschen Aschen left a comment

Choose a reason for hiding this comment

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

Good job 👍 Just a few suggestions on API names
And also for the label, it's more an enhancement to me (or even a bugfix) rather than a new feature

CustomFields map[string]interface{} `json:"-"`
}

func (kr *KuzzleRequest) AddCustomField(k string, v interface{}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In our terminology we call these top level query fields "args" because you can then find them in the request.input.args property.
Maybe a method like AddArg would be more appropriate

Match string `json:"match,omitempty"`
Reset bool `json:"reset,omitempty"`
IncludeTrash bool `json:"includeTrash,omitempty"`
CustomFields map[string]interface{} `json:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Same remark as for AddCustomField, something like CustomArgs

@codecov-io
Copy link

codecov-io commented Jul 12, 2019

Codecov Report

Merging #246 into 2-dev will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##            2-dev     #246      +/-   ##
==========================================
+ Coverage   88.86%   88.91%   +0.05%     
==========================================
  Files         244      244              
  Lines        4463     4466       +3     
==========================================
+ Hits         3966     3971       +5     
+ Misses        443      441       -2     
  Partials       54       54
Impacted Files Coverage Δ
kuzzle/query.go 59.45% <100%> (+1.71%) ⬆️
realtime/subscribe.go 76.47% <0%> (+5.88%) ⬆️

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 31ac1ec...082b018. Read the comment docs.

@Aschen Aschen merged commit 0b6e290 into 2-dev Jul 12, 2019
@Aschen Aschen deleted the 245-custom-args-requests branch July 12, 2019 15:01
@jenow jenow mentioned this pull request Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants