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

Fix JSON Marshaling for Millis and Nanos Types #368

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

justinpolygon
Copy link
Contributor

@justinpolygon justinpolygon commented Dec 5, 2023

This PR addresses two bugs #362 & #360 related to the JSON marshaling of Millis and Nanos types. Both users experienced issues where these types were not correctly marshaled into JSON, resulting in empty field in the JSON output.

// Stocks - Aggregates (Bars)
// https://polygon.io/docs/stocks/get_v2_aggs_ticker__stocksticker__range__multiplier___timespan___from___to
// https://github.com/polygon-io/client-go/blob/master/rest/aggs.go
package main

import (
	"context"
	"log"
	"os"
	"time"
	"encoding/json"
	"fmt"

	polygon "github.com/polygon-io/client-go/rest"
	"github.com/polygon-io/client-go/rest/models"
	"github.com/davecgh/go-spew/spew"
)

func main() {

	// init client
	c := polygon.New(os.Getenv("POLYGON_API_KEY"))

	// set params
	params := models.ListAggsParams{
		Ticker:     "AAPL",
		Multiplier: 1,
		Timespan:   "day",
		From:       models.Millis(time.Date(2023, 12, 1, 0, 0, 0, 0, time.UTC)),
		To:         models.Millis(time.Date(2023, 12, 1, 0, 0, 0, 0, time.UTC)),
	}.WithOrder(models.Desc).WithLimit(50000).WithAdjusted(true)

	// make request
	iter := c.ListAggs(context.Background(), params)

	// do something with the result
	for iter.Next() {
		//log.Print(iter.Item())

		bytes, err := json.Marshal(iter.Item())
		fmt.Println(string(bytes))
		fmt.Println(err)

		// dump the struct
		spew.Dump(iter.Item())

		// Convert Nanos to time.Time
		timestamp := time.Time(iter.Item().Timestamp) //res.Results.ParticipantTimestamp)
		//timestamp := iter.Item().Timestamp

		// Print the Unix millisecond timestamp (int64)
		log.Printf("Unix millisecond timestamp: %d", timestamp.UnixMilli())

		// Print the datetime
		log.Printf("Datetime: %v", timestamp)

	}
	if iter.Err() != nil {
		log.Fatal(iter.Err())
	}

}

Here's the incorrectly marshaled JSON response with the missing t value.

{"c":189.95,"h":190.32,"l":188.19,"n":486786,"o":189.84,"t":{},"v":48744366,"vw":189.337}
<nil>
(models.Agg) {
 Ticker: (string) "",
 Close: (float64) 189.95,
 High: (float64) 190.32,
 Low: (float64) 188.19,
 Transactions: (int64) 486786,
 Open: (float64) 189.84,
 Timestamp: (models.Millis) {
  wall: (uint64) 0,
  ext: (int64) 63836917200,
  loc: (*time.Location)(0x1054207e0)(Local)
 },
 Volume: (float64) 4.8744366e+07,
 VWAP: (float64) 189.337,
 OTC: (bool) false
}
2023/12/04 16:53:24 Unix millisecond timestamp: 1701320400000
2023/12/04 16:53:24 Datetime: 2023-11-29 21:00:00 -0800 PST

The core issue was rooted in the use of pointer receivers (*Millis and *Nanos) for the MarshalJSON methods in these types. In our response structs, for Agg, Trades, Quotes, Snapshot, etc, Millis and Nanos were utilized as non-pointer fields (e.g. Timestamp Millis in the response struct). Consequently, when json.Marshal was called, the custom MarshalJSON methods were not being triggered because of the pointer receiver vs. value receiver miss-match, leading to the observed marshaling problems. I was able to debug this by adding fmt.Println statements to MarshalJSON for both Millis and Nanos and discovered these were never being triggered.

To resolve this, the MarshalJSON methods for both Millis and Nanos have been modified to use value receivers instead of pointer receivers. This change ensures that these methods are appropriately invoked during JSON marshaling, even when Millis and Nanos are used as non-pointer fields. This update aligns the method definitions with the actual usage of these types in our response structs and ensures correct JSON serialization behavior.

Here's the correctly marshaled JSON response with t having the correct value after re-running the script above.

{"c":189.95,"h":190.32,"l":188.19,"n":486786,"o":189.84,"t":1701320400000,"v":48744366,"vw":189.337}
<nil>
(models.Agg) {
 Ticker: (string) "",
 Close: (float64) 189.95,
 High: (float64) 190.32,
 Low: (float64) 188.19,
 Transactions: (int64) 486786,
 Open: (float64) 189.84,
 Timestamp: (models.Millis) {
  wall: (uint64) 0,
  ext: (int64) 63836917200,
  loc: (*time.Location)(0x100fd87e0)(Local)
 },
 Volume: (float64) 4.8744366e+07,
 VWAP: (float64) 189.337,
 OTC: (bool) false
}
2023/12/04 16:51:12 Unix millisecond timestamp: 1701320400000
2023/12/04 16:51:12 Datetime: 2023-11-29 21:00:00 -0800 PST

This update specifically targets the JSON marshaling process and should not affect other areas of the application where Millis and Nanos are used. I ran though all the examples for stocks, options, indices, forex, and crypto without issue.

Copy link

@mmoghaddam385 mmoghaddam385 left a comment

Choose a reason for hiding this comment

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

Good catch, and thanks for the thorough explanation 👍

I was surprised and looked into it some more and found a more formalized explanation for this behavior in the Go spec if you're curious:

  • The method set of a defined type T consists of all methods declared with receiver type T.
  • The method set of a pointer to a defined type T (where T is neither a pointer nor an interface) is the set of all methods declared with receiver *T or T.

So defining MarshalJSON on the concrete type will make it accessible to both the concrete and pointer type.

@justinpolygon
Copy link
Contributor Author

Good catch, and thanks for the thorough explanation 👍

I was surprised and looked into it some more and found a more formalized explanation for this behavior in the Go spec if you're curious.

Great, thanks. Yeah, I was really surprised too.

@justinpolygon justinpolygon merged commit 5511c91 into master Dec 5, 2023
16 checks passed
@justinpolygon justinpolygon deleted the jw-marshal-millis-nanos branch December 5, 2023 16:19
Adam-Mustafa pushed a commit to FinTronners/polygon-client-go that referenced this pull request Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants