DiceDB should not return golang errors to users

[JyotinderSingh]

We return golang errors to users in some of the error paths in our code, largely in functions defined inside eval.go.

This is undesirable. We should only be returning custom error messages that are designed by us to the users, instead of exposing the underlying language details.

As a part of this issue, you will need to identify places where we might be returning language specific errors to the users, and replace them with custom dicedb error messages.

[benjaminmishra]

Hi @JyotinderSingh would love to pick this up.

[BadriVishalPadhy]

Hey @JyotinderSingh i would love to resolve this issue

[JyotinderSingh]

Hi @JyotinderSingh would love to pick this up.

Assigned.

[pree04]

@JyotinderSingh Please assign me some issue to work on . I want to contribute .

[benjaminmishra]

Hi @JyotinderSingh , Just to make sure I understand this correctly, follwing is a code snippet from the evalRPUSH in eval.go for your reference. Here I noticed that we are using return clientio.Encode(err, false) instead of using diceerror. Would you say this that this one example of what we need to correct ?

if err := object.AssertType(obj.TypeEncoding, object.ObjTypeByteList); err != nil {
	return clientio.Encode(err, false)
}

if err := object.AssertEncoding(obj.TypeEncoding, object.ObjEncodingDeque); err != nil {
	return clientio.Encode(err, false)
}

[JyotinderSingh]

Hi @JyotinderSingh , Just to make sure I understand this correctly, follwing is a code snippet from the evalRPUSH in eval.go for your reference. Here I noticed that we are using return clientio.Encode(err, false) instead of using diceerror. Would you say this that this one example of what we need to correct ?

if err := object.AssertType(obj.TypeEncoding, object.ObjTypeByteList); err != nil {

	return clientio.Encode(err, false)

}



if err := object.AssertEncoding(obj.TypeEncoding, object.ObjEncodingDeque); err != nil {

	return clientio.Encode(err, false)

}

Not exactly, the assertType method internally returns a custom error - which is okay.

We are looking to fix instances where we may be seeing golang throw an error and returning the same error back to the user. For instance, say somewhere we try to convert a random string to a number using the ParseFloat API and the operation fails, we should return golang's error message but rather our own (ideally similar to what redis returns)

[JyotinderSingh]

Hi @JyotinderSingh , Just to make sure I understand this correctly, follwing is a code snippet from the evalRPUSH in eval.go for your reference. Here I noticed that we are using return clientio.Encode(err, false) instead of using diceerror. Would you say this that this one example of what we need to correct ?

if err := object.AssertType(obj.TypeEncoding, object.ObjTypeByteList); err != nil {

	return clientio.Encode(err, false)

}



if err := object.AssertEncoding(obj.TypeEncoding, object.ObjEncodingDeque); err != nil {

	return clientio.Encode(err, false)

}

Not exactly, the assertType method internally returns a custom error - which is okay.

We are looking to fix instances where we may be seeing golang throw an error and returning the same error back to the user. For instance, say somewhere we try to convert a random string to a number using the ParseFloat API and the operation fails, we should not return golang's error message but rather our own (ideally similar to what redis returns)

[nehaal10]

hey @JyotinderSingh as I see this issue is still open can you assign it to me so that I can start working on it ?

[JyotinderSingh]

hey @JyotinderSingh as I see this issue is still open can you assign it to me so that I can start working on it ?

Assigned

[JyotinderSingh]

hey @JyotinderSingh as I see this issue is still open can you assign it to me so that I can start working on it ?

Assigned

[nehaal10]

hey @JyotinderSingh in some places like this

func getStringValueAsByteSlice(obj *object.Obj) ([]byte, error) {
	switch obj.Type {
	case object.ObjTypeInt:
		intVal, ok := obj.Value.(int64)
		if !ok {
			return nil, errors.New("expected integer value but got another type")
		}

in this example the its returning golang error we need to replace this custom error like diceerrors.ErrGeneral(<Some Text>)?

just wanted to confirm if I am going in rite direction

[incognito1729]

Hello @JyotinderSingh, If I have understood correctly, such errors should be customized.

For example let's take this function from Eval

// serialize encodes the CountMinSketch into a byte slice.
func (c *CountMinSketch) serialize(buffer *bytes.Buffer) error {
	if c == nil {
		return errors.New("cannot serialize a nil CountMinSketch")
	}

	// Write depth, width, and count
	if err := binary.Write(buffer, binary.BigEndian, c.opts.depth); err != nil {
		return err
	}
	if err := binary.Write(buffer, binary.BigEndian, c.opts.width); err != nil {
		return err
	}
	if err := binary.Write(buffer, binary.BigEndian, c.count); err != nil {
		return err
	}

	// Write matrix
	for i := 0; i < len(c.matrix); i++ {
		for j := 0; j < len(c.matrix[i]); j++ {
			if err := binary.Write(buffer, binary.BigEndian, c.matrix[i][j]); err != nil {
				return err
			}
		}
	}

	return nil
}

instead of directly returning err, we can have

// Write depth, width, and count
if err := binary.Write(buffer, binary.BigEndian, c.opts.depth); err != nil {
	return fmt.Errorf("failed to write CountMinSketch depth (%d): serialization error", c.opts.depth)
}