fix: prevent misuse of transformOptions (#389)

Author: canstand

browser.go

@@ -76,7 +76,7 @@ func (b *browserImpl) NewContext(options ...BrowserNewContextOptions) (BrowserCo
 		options[0].RecordHarContent = nil
 		options[0].RecordHarOmitContent = nil
 	}
-	channel, err := b.channel.Send("newContext", overrides, options)
+	channel, err := b.channel.Send("newContext", options, overrides)
 	if err != nil {
 		return nil, fmt.Errorf("could not send message: %w", err)
 	}

browser_type.go

@@ -23,7 +23,7 @@ func (b *browserTypeImpl) Launch(options ...BrowserTypeLaunchOptions) (Browser,
 		overrides["env"] = serializeMapToNameAndValue(options[0].Env)
 		options[0].Env = nil
 	}
-	channel, err := b.channel.Send("launch", overrides, options)
+	channel, err := b.channel.Send("launch", options, overrides)
 	if err != nil {
 		return nil, fmt.Errorf("could not send message: %w", err)
 	}
@@ -79,7 +79,7 @@ func (b *browserTypeImpl) LaunchPersistentContext(userDataDir string, options ..
 			options[0].RecordHarOmitContent = nil
 		}
 	}
-	channel, err := b.channel.Send("launchPersistentContext", overrides, options)
+	channel, err := b.channel.Send("launchPersistentContext", options, overrides)
 	if err != nil {
 		return nil, fmt.Errorf("could not send message: %w", err)
 	}
@@ -92,7 +92,7 @@ func (b *browserTypeImpl) Connect(wsEndpoint string, options ...BrowserTypeConne
 		"wsEndpoint": wsEndpoint,
 	}
 	localUtils := b.connection.LocalUtils()
-	pipe, err := localUtils.channel.SendReturnAsDict("connect", overrides, options)
+	pipe, err := localUtils.channel.SendReturnAsDict("connect", options, overrides)
 	if err != nil {
 		return nil, err
 	}
@@ -138,7 +138,7 @@ func (b *browserTypeImpl) ConnectOverCDP(endpointURL string, options ...BrowserT
 			options[0].Headers = nil
 		}
 	}
-	response, err := b.channel.SendReturnAsDict("connectOverCDP", overrides, options)
+	response, err := b.channel.SendReturnAsDict("connectOverCDP", options, overrides)
 	if err != nil {
 		return nil, err
 	}

fetch.go

@@ -35,7 +35,7 @@ func (r *apiRequestImpl) NewContext(options ...APIRequestNewContextOptions) (API
 		}
 	}
 
-	channel, err := r.channel.Send("newRequest", overrides, options)
+	channel, err := r.channel.Send("newRequest", options, overrides)
 	if err != nil {
 		return nil, fmt.Errorf("could not send message: %w", err)
 	}
@@ -178,7 +178,7 @@ func (r *apiRequestContextImpl) innerFetch(url string, request Request, options
 		}
 	}
 
-	response, err := r.channel.Send("fetch", overrides, options)
+	response, err := r.channel.Send("fetch", options, overrides)
 	if err != nil {
 		return nil, err
 	}

helpers.go

@@ -62,7 +62,7 @@ func transformStructIntoMapIfNeeded(inStruct interface{}) map[string]interface{}
 		for i := 0; i < v.NumField(); i++ {
 			fi := typ.Field(i)
 			// Skip the values when the field is a pointer (like *string) and nil.
-			if !skipFieldSerialization(v.Field(i)) {
+			if fi.IsExported() && !skipFieldSerialization(v.Field(i)) {
 				// We use the JSON struct fields for getting the original names
 				// out of the field.
 				tagv := fi.Tag.Get("json")
@@ -97,10 +97,14 @@ func transformOptions(options ...interface{}) map[string]interface{} {
 		base = make(map[string]interface{})
 		option = options[0]
 	} else if len(options) == 2 {
-		// Case 3: two values are given. The first one needs to be a map and the
-		// second one can be a struct or map. It will be then get merged into the first
+		// Case 3: two values are given. The first one needs to be transformed
+		// to a map, the sencond one will be then get merged into the first
 		// base map.
-		base = transformStructIntoMapIfNeeded(options[0])
+		if reflect.ValueOf(options[0]).Kind() != reflect.Map {
+			base = transformOptions(options[0])
+		} else {
+			base = transformStructIntoMapIfNeeded(options[0])
+		}
 		option = options[1]
 	}
 	v := reflect.ValueOf(option)

helpers_test.go

@@ -10,9 +10,10 @@ type testOptionsJSONSerialization struct {
 	StringPointer  *string `json:"stringPointer"`
 	NormalString   string  `json:"normalString"`
 	WithoutJSONTag string
-	WithJSONTag    string  `json:"withJSONTag"`
-	SkipNilPtrs    *string `json:"skipNilPtrs"`
-	SkipMe         *int    `json:"skipMe"`
+	WithJSONTag    string   `json:"withJSONTag"`
+	OverrideMe     []string `json:"overrideMe"`
+	SkipNilPtrs    *string  `json:"skipNilPtrs"`
+	SkipMe         *int     `json:"skipMe"`
 }
 
 func TestTransformOptions(t *testing.T) {
@@ -22,17 +23,18 @@ func TestTransformOptions(t *testing.T) {
 		NormalString:   "2",
 		WithoutJSONTag: "3",
 		WithJSONTag:    "4",
+		OverrideMe:     []string{"5"},
 	}
 	var nilStrPtr *string
 	testCases := []struct {
-		name           string
-		baseMap        map[string]interface{}
-		optionalStruct interface{}
-		expected       interface{}
+		name         string
+		firstOption  interface{}
+		secondOption interface{}
+		expected     interface{}
 	}{
 		{
 			name: "No options supplied",
-			baseMap: map[string]interface{}{
+			firstOption: map[string]interface{}{
 				"1234": nilStrPtr,
 				"foo":  "bar",
 			},
@@ -42,44 +44,60 @@ func TestTransformOptions(t *testing.T) {
 		},
 		{
 			name: "Options are nil",
-			baseMap: map[string]interface{}{
+			firstOption: map[string]interface{}{
 				"foo": "bar",
 			},
-			optionalStruct: nil,
+			secondOption: nil,
 			expected: map[string]interface{}{
 				"foo": "bar",
 			},
 		},
 		{
 			name: "JSON serialization works",
-			baseMap: map[string]interface{}{
-				"foo": "bar",
+			firstOption: map[string]interface{}{
+				"foo":           "bar",
+				"stringPointer": 1,
 			},
-			optionalStruct: structVar,
+			secondOption: structVar,
 			expected: map[string]interface{}{
 				"foo":            "bar",
 				"stringPointer":  String("1"),
 				"normalString":   "2",
 				"WithoutJSONTag": "3",
 				"withJSONTag":    "4",
+				"overrideMe":     []interface{}{"5"},
 			},
 		},
 		{
 			name: "Second overwrites the first one",
-			baseMap: map[string]interface{}{
+			firstOption: map[string]interface{}{
 				"foo": "1",
 			},
-			optionalStruct: map[string]interface{}{
+			secondOption: map[string]interface{}{
 				"foo": "2",
 			},
 			expected: map[string]interface{}{
 				"foo": "2",
 			},
 		},
+		{
+			name:        "Second overwrites the first one's value in different type",
+			firstOption: structVar,
+			secondOption: map[string]interface{}{
+				"overrideMe": "5",
+			},
+			expected: map[string]interface{}{
+				"stringPointer":  String("1"),
+				"normalString":   "2",
+				"WithoutJSONTag": "3",
+				"withJSONTag":    "4",
+				"overrideMe":     "5",
+			},
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			require.Equal(t, tc.expected, transformOptions(tc.baseMap, tc.optionalStruct))
+			require.Equal(t, tc.expected, transformOptions(tc.firstOption, tc.secondOption))
 		})
 	}
 }

page.go

@@ -337,7 +337,6 @@ func (p *pageImpl) Screenshot(options ...PageScreenshotOptions) ([]byte, error)
 				}
 			}
 			overrides["mask"] = masks
-			options[0].Mask = nil
 		}
 	}
 	data, err := p.channel.Send("screenshot", options, overrides)
@@ -358,7 +357,7 @@ func (p *pageImpl) Screenshot(options ...PageScreenshotOptions) ([]byte, error)
 
 func (p *pageImpl) PDF(options ...PagePdfOptions) ([]byte, error) {
 	var path *string
-	if len(options) > 0 {
+	if len(options) == 1 {
 		path = options[0].Path
 	}
 	data, err := p.channel.Send("pdf", options)

tests/page_test.go

@@ -97,6 +97,7 @@ func TestPageScreenshotWithMask(t *testing.T) {
 		Mask: []playwright.Locator{
 			sensElem,
 		},
+		MaskColor: playwright.String("red"),
 	})
 	require.NoError(t, err)
 	require.True(t, filetype.IsImage(screenshot2))