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

MYST-39 Deserialise ServiceProposal DTOs from JSON #36

Merged
merged 11 commits into from Oct 17, 2017

Conversation

Projects
None yet
3 participants
@Waldz
Copy link
Member

commented Oct 7, 2017

No description provided.

PaymentMethodPerTime{},
`{
"price": {
"amount": 0,

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member
  • Now it looks like free amount
  • But actually it's unset amount e.g. "price": {}
  • I would use omitempty
log.Fatal(err)
}

ovProposal := dto.ServiceProposal{

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member

Whayis ov?

ProviderContacts []dto.Contact `json:"provider_contacts"`
}

func BuildServiceProposalFromJson(jsonData []byte) dto.ServiceProposal {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member

This function can be refactored to custom unmarshaler on ServiceProposal object

},
{
`{
"price": {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member

I think empty price should be {} but not free 0 price


import (
"testing"
//"fmt"

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member

clean

}

func BuildServiceProposalFromJson(jsonData []byte) dto.ServiceProposal {
proposal := proposal{}

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member

Your skip value assignment with var proposal proposal

},
LocationOriginate: dto.Location{
Country: "US",
City: "",

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member

Should not those be unset values?

Amount: 10000000,
Currency: money.Currency("MYST"),
},
Duration: time.Duration(3600),

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member

1 * time.Hour or 60 * time.Minute


switch proposal.ServiceType {
case "openvpn":
ovProposal.ServiceDefinition = getServiceDefinitionFromJson(proposal.ServiceDefinition)

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member

I think it's possible to stop depending on Openvpn implementation in this switcher:

DTODiscovery would be type agnostic:

type serviceDefinitionUnserializer func(*RawMessage) ServiceDefinition

var serviceDefinitionMap map[string]serviceDefinitionUnserializer

func RegisterServiceDefinition(serviceType string, unserializer serviceDefinitionUnserializer) {
    serviceDefinitionMap[serviceType] = unserializer
}

DTOOpenvpn hooks on top:

dto_discovery.RegisterServiceDefinition("openvpn", func(rawDefinition *json.RawMessage) ServiceDefinition {
	var definition dto_openvpn.ServiceDefinition
	json.Unmarshal(*rawDefinition, &definition)

	return definition
}
@Waldz
Copy link
Member Author

left a comment

More

"service_definition": {
"location": {
"country": "US",
"city": "Washington DC",

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member

No need to add full objects, because they are very much tested already.
Would be enough having 1st level only:

"format": "service-proposal/v1",
"service_type": "openvpn",
"service_definition": {
    "location": {"country": "US"},
},
"payment_method_type": "PER_BYTES",
"payment_method": {"bytes": 1}
Amount: 50000000,
Currency: money.Currency("MYST"),
},
Bytes: datasize.BitSize(8589934592),

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member

Cuuld be readable format like 1 * datasize.Gygabyte

{
PaymentMethodPerBytes{
Price: price,
Bytes: datasize.Gigabyte,

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member

Field Bytes is not in Bytes anymore.
Should we rename it to throughput or bitsize?
https://en.wikipedia.org/wiki/Data_rate_units

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Oct 17, 2017

Member

What about a generic "threshold". Could use it for time as well and based on the context of the struct name you'll know what it's about.

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Oct 17, 2017

Member

Maybe change Bytes to Amount and Duration to Amount? I think it would make sense to add GetAmount() method on the PaymentMethod interface.

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 17, 2017

Author Member

I liked something:

  • PaymentMethodPerBytes -> PaymentMethodPerData raname struct also
  • Bytes -> Amount
  • or Bytes -> Size
@Waldz
Copy link
Member Author

left a comment

More on error handling

definition := ServiceDefinition{}

if err := json.Unmarshal(*rawDefinition, &definition); err != nil {
log.Fatal(err)

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member

You dont know how to handle this error in this low layer, so error should be pushed to upper layer:

func getServiceDefinitionFromJson() (ServiceDefinition, error) {
    if err := json.Unmarshal(*rawDefinition, &definition); err != nil {
	return nil, err
   }
}
proposal := proposal{}

if err := json.Unmarshal([]byte(jsonData), &proposal); err != nil {
log.Fatal(err)

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 16, 2017

Author Member

Error should pushed to upper layer also BuildServiceProposalFromJson() (dto.ServiceProposal, error)

@Waldz Waldz merged commit b96b313 into master Oct 17, 2017

@Waldz Waldz deleted the feature/MYST-39-dto-serialize branch Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.