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

Stop using JSON across Go/Swift boundary and use pathdb's built-in support for protobuf #930

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

oxtoacart
Copy link

@oxtoacart oxtoacart commented Oct 11, 2023

I noticed that we were using JSON to send subscription results from Go to Swift. In addition to having some overhead, this also complicated the code a bit. This PR removes JSON in favor of just using Go objects that can be bound with gomobile.

The other change that this makes is to rely on pathdb's built-in support for protocol buffers. That built-in support wasn't complete, so I made some updates to pathdb, notably the Raw.ValueOrProtoBytes() method. Going forward, when we want to store new protobuf types in the database, we just need to register them like this

base.db.RegisterType(1000, &ServerInfo{})

You'll notice that the existing Android application follows this same pattern with db-android.

Saved 154 lines of code too :)

@oxtoacart oxtoacart requested a review from jigar-f October 11, 2023 02:31
@oxtoacart oxtoacart force-pushed the ox/ios-migrate/pathdb-changes branch from 8c37efa to 8008c1d Compare October 11, 2023 02:34
@oxtoacart oxtoacart changed the title Stop using JSON across Go/Swift boundary Stop using JSON across Go/Swift boundary and use pathdb's built-in support for protobuf Oct 11, 2023
err := pathdb.Mutate(m.db, func(tx pathdb.TX) error {
pathdb.Put[string](tx, SERVER_COUNTRY, p1, "")
pathdb.Put[string](tx, SERVER_CITY, p0, "")
pathdb.Put[string](tx, SERVER_COUNTRY_CODE, p2, "")
pathdb.Put[bool](tx, HAS_SUCCEEDING_PROXY, p5, "")
pathdb.Put[[]byte](tx, PATH_SERVER_INFO, serverInfoBytes, "")
pathdb.Put[*ServerInfo](tx, PATH_SERVER_INFO, serverInfo, "")
Copy link
Author

Choose a reason for hiding this comment

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

Because we registered *ServerInfo with pathdb, we can directly Put it with having to marshal it ourselves. We don't do this, but we could also directly Get it without having to handle the protocol buffer unmarshalling ourselves.

@jigar-f
Copy link
Contributor

jigar-f commented Oct 11, 2023

Thanks @oxtoacart, I did see this in Kotlin, But I thought this was not supported on pathDb, Thanks for letting me know.

@jigar-f
Copy link
Contributor

jigar-f commented Oct 11, 2023

I am going to test this PR and let you know the result.

@oxtoacart
Copy link
Author

this was not supported on pathDb

It wasn't fully supported, but it was supposed to be, so I fixed pathdb.

Copy link
Contributor

@jigar-f jigar-f left a comment

Choose a reason for hiding this comment

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

LGTM!

@jigar-f
Copy link
Contributor

jigar-f commented Oct 11, 2023

@oxtoacart All changes are working fine, If you are done with PR, I like to merge it.

@oxtoacart oxtoacart merged commit fc02012 into ios-migrate Oct 11, 2023
1 of 3 checks passed
@oxtoacart oxtoacart deleted the ox/ios-migrate/pathdb-changes branch October 11, 2023 15:45
@oxtoacart
Copy link
Author

Thanks @jigar-f !

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