feat: Memconfig client #301
Conversation
client/memconfig.go
Outdated
| var cfg intermediaryCfg | ||
| if err := json.NewDecoder(resp.Body).Decode(&cfg); err != nil { |
There was a problem hiding this comment.
if we implement the custom UnmarshalJSON on memconfig directly we wouldn't need this afaiu
client/memconfig.go
Outdated
| // UpdateConfig updates the configuration using the new MemConfig instance | ||
| // by sending a PATCH request to the proxy server | ||
| func (c *MemCfgClient) UpdateConfig(ctx context.Context, update *MemConfig) (*MemConfig, error) { | ||
| body, err := update.MarshalJSON() |
There was a problem hiding this comment.
This is a bit dangerous the way its implemented, because it doesn't really allow you to PATCH. You should prob use omitempty tag in the struct. Otherwise any field that is not initialized will be given zero value and happily overwrite the server's config with those values. We want to make sure zero fields are not included in the marshalled json (except for boolean values...!)
There was a problem hiding this comment.
hmmm how would fields be omitted if we're passing in a structured MemConfig to update? This would make more sense if we providing an arbitrary key/value interface which we're not. This client functionality should work fine for the use case of Arbitrum E2E testing and shouldn't result in fields being discarded.
There was a problem hiding this comment.
The problem is that because it's a PATCH, someone could pass in
{
PutReturnsFailoverError: true
}
thinking they'll just update this single value, but actually what this will do is set all the other fields to their zero value.
This is why I'm suggesting instead of this method taking in a *MemConfig, it instead takes a ConfigUpdate like I used in the proxy side.
There was a problem hiding this comment.
There are 2 other options that you might prefer:
- we forget that I implemented this as a PATCH, and we add a big disclaimer in the comment to this method saying that this method is actually implemented as a PUT, where the EXACT MemConfig passed as input will be set on the proxy. We then suggest that a client gets the current MemConfig first, and then make modifications to that before calling UpdateConfig.
- We define individual setters for each field which will create a patch with only that specific field. This might be cleaner actually.. with the downside that if you need to update multiple fields at once you'll need to make N calls.
There was a problem hiding this comment.
would prefer to just keep it as is. currently we only plan to use this in the arbitrum integration for testing failover. reading the entire config to update a specific field should be ok for the existing use case.
There was a problem hiding this comment.
Fine with me, although I PROMISE you that this will cause a bug at some point.
Can you at least add the disclaimer comment I suggested in option 1 above then?
client/memconfig.go
Outdated
| // UpdateConfig updates the configuration using the new MemConfig instance | ||
| // by sending a PATCH request to the proxy server | ||
| func (c *MemCfgClient) UpdateConfig(ctx context.Context, update *MemConfig) (*MemConfig, error) { | ||
| body, err := update.MarshalJSON() |
There was a problem hiding this comment.
The problem is that because it's a PATCH, someone could pass in
{
PutReturnsFailoverError: true
}
thinking they'll just update this single value, but actually what this will do is set all the other fields to their zero value.
This is why I'm suggesting instead of this method taking in a *MemConfig, it instead takes a ConfigUpdate like I used in the proxy side.
client/memconfig.go
Outdated
| // UpdateConfig updates the configuration using the new MemConfig instance | ||
| // by sending a PATCH request to the proxy server | ||
| func (c *MemCfgClient) UpdateConfig(ctx context.Context, update *MemConfig) (*MemConfig, error) { | ||
| body, err := update.MarshalJSON() |
There was a problem hiding this comment.
There are 2 other options that you might prefer:
- we forget that I implemented this as a PATCH, and we add a big disclaimer in the comment to this method saying that this method is actually implemented as a PUT, where the EXACT MemConfig passed as input will be set on the proxy. We then suggest that a client gets the current MemConfig first, and then make modifications to that before calling UpdateConfig.
- We define individual setters for each field which will create a patch with only that specific field. This might be cleaner actually.. with the downside that if you need to update multiple fields at once you'll need to make N calls.
| } | ||
|
|
||
| // UpdateConfig updates the configuration using the new MemConfig instance | ||
| // Despite the API using a PATH method, this function treats the "update" config |
There was a problem hiding this comment.
| // Despite the API using a PATH method, this function treats the "update" config | |
| // Despite the API using a PATCH method, this function treats the "update" config |
Fixes Issue
Fixes #294
Changes proposed
Screenshots (Optional)
Note to reviewers