sewenew / redis-protobuf

Redis module for reading and writing Protobuf messages
Apache License 2.0
204 stars 22 forks source link

redis crash on setting repeated field by index #24

Closed mhaberler closed 3 years ago

mhaberler commented 3 years ago

used proto file

commands:

127.0.0.1:6379> FLUSHALL
OK

127.0.0.1:6379> pb.append test FeatureCollection.features  '{geometry: {point: {coordinates: [1,2,3]}}}'
(integer) 1

127.0.0.1:6379> pb.get test --format json FeatureCollection
"{\"features\":[{\"geometry\":{\"point\":{\"coordinates\":[1,2,3]}}}]}"

127.0.0.1:6379> pb.set test  FeatureCollection.features[0].geometry.point '{coordinates: [4,5,2]}'
(integer) 1

# unclear how to do this?
127.0.0.1:6379> pb.set test  FeatureCollection.features[0].geometry.point.coordinates '[4,5,2]'
(error) ERR cannot set the whole array field

# try to set the first value in the coordinates array:

127.0.0.1:6379> pb.set test  FeatureCollection.features[0].geometry.point.coordinates[0] 4711
.... redis crashes ...
Could not connect to Redis at 127.0.0.1:6379: Connection refused

full crashlog is here, the key lines:

[libprotobuf FATAL google/protobuf/generated_message_reflection.cc:103] Protocol Buffer reflection usage error:
  Method      : google::protobuf::Reflection::SetFloat
  Message type: Point
  Field       : Point.coordinates
  Problem     : Field is repeated; the method requires a singular field.
libc++abi.dylib: terminating with uncaught exception of type google::protobuf::FatalException: Protocol Buffer reflection usage error:
  Method      : google::protobuf::Reflection::SetFloat
  Message type: Point
  Field       : Point.coordinates
  Problem     : Field is repeated; the method requires a singular field.
sewenew commented 3 years ago

Sorry, the crash is a bug. I've fixed it. Please try the latest code on master.

unclear how to do this?

So far, redis-protobuf does not support setting a whole array or map field, i.e. replace the array with other one. And there's an issue to track it.

Sorry again for the inconvenience...

Regards

mhaberler commented 3 years ago

no problem, thanks for the quick fix!

mhaberler commented 3 years ago

verified the fix - great job!

sewenew commented 3 years ago

By the way, it seems that you used the oneof feature. However, I didn't write specific code to process this feature.

Although in your case, it works, I'm not sure if it has any problem. If you want to avoid this uncertainty, you can do some change with your protobuf definition:

Change this:

message Geometry {
  oneof type {
    Point point = 1;
    MultiPoint multiPoint = 2;
    LineString lineString = 3;
    MultiLineString multiLineString = 4;
    Polygon polygon = 5;
    MultiPolygon multiPolygon = 6;
    GeometryCollection geometryCollection = 7;
  }
}

into:

message Geometry {
    Point point = 1;
    MultiPoint multiPoint = 2;
    LineString lineString = 3;
    MultiLineString multiLineString = 4;
    Polygon polygon = 5;
    MultiPolygon multiPolygon = 6;
    GeometryCollection geometryCollection = 7;
}

Regards

mhaberler commented 3 years ago

so far no problems, will have an eye on it. Thanks!