aerospike / aerospike-client-go

Aerospike Client Go
Apache License 2.0
430 stars 198 forks source link

Using Client.GetObject fails with "interface conversion: interface {} is bool, not int" #325

Closed EmmaSimon closed 3 years ago

EmmaSimon commented 3 years ago

I'm trying to use GetObject to marshal the bin data into a struct containing a handful of bool values. The error comes from the lines below in read_command_reflect.go, where value is apparently already a bool, so trying to convert it to an int fails.

case reflect.Bool:
    f.SetBool(value.(int) == 1)

I assume this could also happen with the other more complicated case reflect.Bool in the switch under the reflect.Ptr case further down.

When I replace the first bool case with the following, everything seems to work:

case reflect.Bool:
    switch v := value.(type) {
    case int:
        f.SetBool(int(v) == 1)
    default:
        f.SetBool(bool(value.(bool)))
    }

I'm not sure if this is an optimal solution, but I'm happy to put up a PR if it seems okay.

khaf commented 3 years ago

Interesting. Could you share the struct that you are trying to unmarshal to? I'm not sure how the database would return a bool value. Also, is it possible to share the values of the Record when retrieved via a simple Get call?

EmmaSimon commented 3 years ago

Here's the struct(s):

type EventRegistration struct {
  Category        string                   `json:"category" as:"category"`               
  CreationDate    string                   `json:"creationDate" as:"creationDate"`       
  DeprecationDate string                   `json:"deprecationDate" as:"deprecationDate"` 
  Doc             string                   `json:"doc" as:"doc"`                         
  EventType       string                   `json:"eventType" as:"eventType"`             
  Fields          []EventRegistrationField `json:"fields" as:"fields"`                   
  Implements      []string                 `json:"implements" as:"implements"`           
  IsDeprecated    bool                     `json:"isDeprecated" as:"isDeprecated"`       
  IsPublic        bool                     `json:"isPublic" as:"isPublic"`               
  IsRealtime      bool                     `json:"isRealtime" as:"isRealtime"`           
  Labels          []string                 `json:"labels" as:"labels"`                   
  Name            string                   `json:"name" as:"name"`                       
  Namespace       string                   `json:"namespace" as:"namespace"`             
  OwnerId         int64                    `json:"ownerId" as:"ownerId"`                 
  SlackChannel    string                   `json:"slackChannel" as:"slackChannel"`       
  Transcription   []string                 `json:"transcription" as:"transcription"`     
}

type EventRegistrationField struct {
  DataType     string                       `json:"dataType" as:"dataType"`         
  Doc          string                       `json:"doc" as:"doc"`                   
  FieldSource  EventRegistrationFieldSource `json:"fieldSource" as:"fieldSource"`   
  FieldType    string                       `json:"fieldType" as:"fieldType"`       
  IsDeprecated bool                         `json:"isDeprecated" as:"isDeprecated"` 
  IsRequired   bool                         `json:"isRequired" as:"isRequired"`     
  Name         string                       `json:"name" as:"name"`                 
}

type EventRegistrationFieldSource struct {
  Backend   []string `json:"backend" as:"backend"`     
  Client    []string `json:"client" as:"client"`       
  Universal []string `json:"universal" as:"universal"` 
}

And record.Bins (sorry it's not pretty printed):

map[doc:This event represents a customer placing an order eventType:model fields:[map[dataType:long doc:Id of the order placed by customer fieldSource:map[] isDeprecated:false isOverridden:false isRequired:true name:orderId validators:[autoIncrementedId]] map[dataType:string doc:Promo code used (if any) during the order fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:promoCode validators:<nil>] map[dataType:list<string> doc:List of products bought fieldSource:map[backend:[trackingLog.listByValues(eventMap,"skus")]] isDeprecated:false isOverridden:false isRequired:false name:skus validators:[skulist]] map[dataType:list<string> doc:The internal Pr (product) SKUs of the product (original SKU format). fieldSource:map[backend:[trackingLog.listByValues(eventMap,"skus")]] isDeprecated:false isOverridden:false isRequired:false name:productSkus validators:[skulist]] map[dataType:list<string> doc:The user-facing display SKUs of the product (new SKU format). fieldSource:map[backend:[trackingLog.listByValues(eventMap,"displaySkus")]] isDeprecated:false isOverridden:false isRequired:false name:displaySkus validators:[skulist]] map[dataType:string doc:The phone number entered by the customer at checkout. fieldSource:map[backend:[trackingLog.billingPhone(eventMap)]] isDeprecated:false isOverridden:false isRequired:true name:billingPhone validators:[phone]] map[dataType:integer doc:The number of items in this order. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:numberOfItems validators:<nil>] map[dataType:price doc:The total price in cents that this order amounts to. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:orderTotal validators:<nil>] map[dataType:string doc:The current user's zip-code. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:zipCode validators:<nil>] map[dataType:map<string> doc:A comma-separated string of the optionIds associated with the SKU. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:productOptions validators:<nil>] map[dataType:map<string> doc:A comma-separated string of the obfuscatedOptionIds associated with the display SKU. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:productObfuscatedOptions validators:<nil>] map[dataType:string doc:Type of visitor new/returning/aquired/activated fieldSource:map[backend:[trackingLog.visitorType]] isDeprecated:true isOverridden:false isRequired:false name:visitorType validators:<nil>] map[dataType:uuid doc:Google Advertising ID for Android devices. fieldSource:map[backend:[trackingLog.adid]] isDeprecated:false isOverridden:false isRequired:false name:adid validators:<nil>] map[dataType:string doc:The billing address of the customer for this order. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:billingAddress validators:<nil>] map[dataType:map<string> doc:A map of each SKU in the order to it's shipping address, if any. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:skuShippingAddresses validators:<nil>] map[dataType:map<string> doc:A map of each SKU in the order to the company associated with it's shipping address, if any. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:skuCompanyNames validators:<nil>] map[dataType:string doc:The first name associated with the billing address of the customer placing this order. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:billingFirstName validators:[customername]] map[dataType:string doc:The last name associated with the billing address of the customer placing this order. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:billingLastName validators:[customername]] map[dataType:string doc:The email address of the customer placing this order. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:emailAddress validators:<nil>] map[dataType:map<string> doc:A map of each SKU in this order to it's unit price and it's quantity as a JSON String. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:skuPriceQuantities validators:<nil>] map[dataType:list<integer> doc:An ordered list of class IDs of the SKUs in this order. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:classIds validators:<nil>] map[dataType:map<boolean> doc:A map of each SKU in this order and if the shipping address associated with it is commercial or not. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:skuShippingAddressCommercial validators:<nil>] map[dataType:map<string> doc:A map of each SKU in this order and the first name associated with it's shipping address. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:skuShippingFirstNames validators:[customernamemap]] map[dataType:map<string> doc:A map of each SKU in this order and the last name associated with it's shipping address. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:skuShippingLastNames validators:[customernamemap]] map[dataType:uuid doc:Wayfair ID for the device. fieldSource:map[backend:[trackingLog.wfDeviceId]] isDeprecated:false isOverridden:false isRequired:false name:wfDeviceId validators:<nil>] map[dataType:string doc:The name of the company associated with the billing address for this order; if any. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:billingCompanyName validators:<nil>] map[dataType:boolean doc:If the billing address associated with this order is commercial or not. fieldSource:map[backend:[trackingLog.parseBooleanPrimitive(eventMap,"isBillingAddressCommercial")]] isDeprecated:false isOverridden:false isRequired:false name:isBillingAddressCommercial validators:<nil>] map[dataType:string doc:A string indicating if this order is for a client or business or not, as specified by the customer. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:orPurchaseUse validators:<nil>] map[dataType:integer doc:An integer indicating type of business this order is for. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:orPurchaseUseCoType validators:<nil>] map[dataType:integer doc:An integer indicating the number of employees associated with this business. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:orPurchaseUseCoSize validators:<nil>] map[dataType:string doc:The agent used to make this request. fieldSource:map[backend:[trackingLog.userAgent]] isDeprecated:false isOverridden:false isRequired:false name:userAgent validators:<nil>] map[dataType:IpAddress doc:The IP Address of the client from which the request was made. fieldSource:map[backend:[trackingLog.clientIp]] isDeprecated:false isOverridden:false isRequired:false name:clientIp validators:<nil>] map[dataType:integer doc:The shard number for the shard that the customer data resides on. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:customerShard validators:<nil>] map[dataType:boolean doc:Indicates if the customer placing the order is logged in as a b2b customer or not fieldSource:map[backend:[trackingLog.parseBooleanPrimitive(eventMap,"isB2bCustomer")]] isDeprecated:false isOverridden:false isRequired:false name:isB2bCustomer validators:<nil>] map[dataType:integer doc:Id of the payment method used in this order, corresponds to values in csn_order.dbo.tblplPaymentMethod fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:paymentMethodId validators:<nil>] map[dataType:long doc:Bank identifier for the credit card associated with the provided card number. First 6 digits of CC number. fieldSource:map[] isDeprecated:false isOverridden:false isRequired:false name:bankIdentifierNumber validators:<nil>]] implements:[CustomerSite PageRequestAssociated Tracking] isAutoscribe:1 isDeprecated:0 isPublic:1 isRealtime:1 name:OrderPlaced namespace:scribe.events transcription:[backend] type:model]

I was also confused by the database returning bool values, but it looks like they only exist in nested maps in the fields bin, the top level boolean bins do have the regular 0 and 1s.

khaf commented 3 years ago

Ah this removes the confusion. Aerospike does in fact support booleans in CDTs. I'll take care of the changes, and will release them before the end of next week. Thank you for your report and feedback.

EmmaSimon commented 3 years ago

Great, thank you so much!

khaf commented 3 years ago

OK, I have implemented the functionality, but I can't seem to make a failing test case. Do you think you can provide such test case to verify the implementation?

EmmaSimon commented 3 years ago

Hm, I'm having trouble too. It's possible that this client is putting CDT bools as ints (they're coming back as ints in the raw bin values), so reading them out works fine. The values I'm pulling were inserted with the Java client, that might handle the CDT puts differently so that the actual value stored in the DB is different. Not sure how it would be possible to make that behavior happen in the tests.

khaf commented 3 years ago

The fix is released in v4.0.0. Feel free to close the ticket if the issue is resolved.

EmmaSimon commented 3 years ago

Looks like it's working now! 🎉 I should maybe make another ticket with this question, but what exactly was the issue with BatchIndexGet that you mentioned in the release notes? I think I'm having a similar problem with BatchGet and BatchGetObjects where not all keys are fetched. In the previous version, it was a specific set (of definitely valid) keys which would always fail to be found, except sometimes if another batch request was made for them earlier during execution. This is the error: Node BB9935EAA565000 10.228.156.75:3000: Connection pool is empty. This happens when either all connection are in-use already, or no connections were available Fetching the same keys individually worked fine. Now, after the upgrade, some subset of keys fails for every BatchGetObjects call for ~450 keys, usually more than half of them, but sometimes less than 100.

khaf commented 3 years ago

The issue was that the the BatchGetComplex calls were one shot and would not retry in some cases. Now they are retried under all circumstances. Keep in mind that Batch commands are rather connection intensive, since they will consume a connection from every node. So you need to increase the number of connections in the pool. It should always work and return every key (under normal network and operating conditions), and if it doesn't, it's a bug and should be investigated and fixes.

khaf commented 3 years ago

Just to add more detail: The Timeout values and retry count can vastly change the results, so it is worth an investigation and some trial and error.

gmaz42 commented 3 years ago

Hm, I'm having trouble too. It's possible that this client is putting CDT bools as ints (they're coming back as ints in the raw bin values), so reading them out works fine. The values I'm pulling were inserted with the Java client, that might handle the CDT puts differently so that the actual value stored in the DB is different.

I am afraid that the fix in 555e1b2be11518f7b8d3901025db54a641ece374 makes the client library more lax on the incoming type from Aerospike server, while the problem was actually as reported that one client is storing ints and the other is expecting bools.

khaf commented 3 years ago

@gmazzotta Why do you think so? This is a reflection API, and things are supposed to work as smoothly as possible. The field is boolean, and only specific values will be converted.

gmaz42 commented 3 years ago

@khaf it sounds like a slippery slope: why not cast float to boolean as well? and so on.

Instead I think that providing consistent APIs is a great benefit, one can say: the type returned from Aerospike is mapped only to the closest matching type, and avoid feature requests to create all sorts of convenient/sloppy type conversions.

Edit: nevermind, I was sure that Aerospike would have a native boolean type, it does not; my comments were made with that assumption in mind.

khaf commented 3 years ago

It is a slippery slope, and that's why there is no string conversion support of "T", "F", "TRUE", "FALSE", "Y", "N", etc to boolean. And believe me, I used to do these in dBase, FoxPro days so I actually thought about them. In this case, we need to keep compatibility between our clients.

gmaz42 commented 3 years ago

I think an automated test using GitHub actions can be arranged in that sense:

khaf commented 3 years ago

It's harder than you think. We have a multitude of client libraries, and these issues are rare corner cases.

gmaz42 commented 3 years ago

Ok, I was only thinking of this one https://www.aerospike.com/docs/client/java/

And I would definitely try to auto-generate at least part of the code