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

proposal: database/sql: ScannerContext, ValuerContext support #40511

Closed
jinzhu opened this issue Jul 31, 2020 · 9 comments
Closed

proposal: database/sql: ScannerContext, ValuerContext support #40511

jinzhu opened this issue Jul 31, 2020 · 9 comments
Labels
Projects
Milestone

Comments

@jinzhu
Copy link

@jinzhu jinzhu commented Jul 31, 2020

Currently when using Scanner, Valuer interface with database/sql, it can't behave differently based on the current context

For example, in a multi-tenant system, each tenant has a different encryption key and would like to save the encrypted value and retrieve the decrypted value automatically.

If we can support ScannerContext and ValuerContext interface in the package database/sql, it would be much easier to implement those cases.

// option 1
type ScannerContext interface {
  Scan(ctx context.Context, src interface{}) error
}

type ValuerContext interface {
	Value(ctx context.Context) (Value, error)
}

// option 2
type ScannerContext interface {
  ScanContext(ctx context.Context, src interface{}) error
}

type ValuerContext interface {
	ValueContext(ctx context.Context) (Value, error)
}
@gopherbot gopherbot added this to the Proposal milestone Jul 31, 2020
@gopherbot gopherbot added the Proposal label Jul 31, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 7, 2020

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Aug 7, 2020
@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 11, 2020

@jinzhu I strongly suspect that this isn't going to fly. But as of yet I don't see a complete example. Perhaps you can flush it out a bit with how you would call this so I can fully understand what you are trying to do.

@jinzhu
Copy link
Author

@jinzhu jinzhu commented Aug 11, 2020

Hello @kardianos

Thank you for your reply, we have many applications have similar requirements.

The application need to save multiple fields of some structs as encrypted data into the database, for each organization, it has an encryption key, we will store the encryption key into context for coming API requests.

There are many services using the data have to encrypt/decrypt those fields one by one everywhere, it is easy to make a mistake when processing the data, e.g, double or forget to encrypt a field that corrupted the data, which will make the application less maintainable.

If Go supports ScannerContext and ValuerContext, we can move the encrypt/decrypt logic to the interface to avoid those issues, which helps deliver a more maintainable product without breaking the go1 compatibility.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 11, 2020

@jinzhu Thank you for the description. Can you write some code as you would envision using this? As before, I doubt this proposal will be acceptable, so set expectations accordingly.

@jinzhu
Copy link
Author

@jinzhu jinzhu commented Aug 12, 2020

@kardianos If it is supported, then we can write our application like this:

type User struct {
	ID      string
	Name    string
	Email   EncryptedString
	Phone   EncryptedString
	Address EncryptedString
}

type Message struct {
	ID   uint
	From uint
	To   uint
	Body EncryptedString
}

type EncryptedString struct {
	encryptedValue string
	decryptedValue string
}

func (es *EncryptedString) ScanContext(ctx context.Context, src interface{}) (err error) {
	if encryptionKey, ok := ctx.Value("TenantEncryptionKey").(string); ok {
		ns := sql.NullString{}
		if err = ns.Scan(src); err == nil && ns.String != "" {
			es.decryptedValue, err = Decrypt(ns.String, encryptionKey)
		}
	} else {
		return errors.New("invalid encryption key")
	}
	return err
}

func (es EncryptedString) ValueContext(ctx context.Context) (_ driver.Value, err error) {
	if encryptionKey, ok := ctx.Value("TenantEncryptionKey").(string); ok {
		if es.encryptedValue != "" {
			return es.encryptedValue, nil
		}

		if es.decryptedValue != "" {
			es.encryptedValue, err = Encrypt(es.decryptedValue, encryptionKey)
		}
	} else {
		return nil, errors.New("invalid encryption key")
	}
	return es.encryptedValue, err
}

func (es *EncryptedString) UnmarshalJSON(b []byte) error {
	json.Unmarshal(b, &es.decryptedValue)
	return nil
}

func (es *EncryptedString) MarshalJSON() ([]byte, error) {
	return []byte(es.decryptedValue), nil
}

and remove the encrypt/decrypt logic which scattered everywhere right now

@jinzhu
Copy link
Author

@jinzhu jinzhu commented Aug 26, 2020

Hello @kardianos

Any plan, can I start to implement this feature?

@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 27, 2020

I don't feel great about this feature request. While the context is usually available in a Scan, it isn't obvious what it is used by.

You can probably achieve a similar result by creating a helper scan stub:

type UpdateWithContext interface {
    Update(ctx context.Context) error
}
func MyAppScan(ctx context.Context, scanner Scanner, values ...interface{}) error {
    err := scanner.Scan(values...)
    if err != nil {
        return err
    }
    for _, v := range values {
        if ok, v := v.(UpdateWithContext); ok {
            err = v.Update(ctx)
            if err != nil {
                return err
            }
        }
    }
    return nil
}

I would like to have a story for this in a v2 with better custom scanning / marshaling, but not in v1.

@kardianos kardianos moved this from Incoming to Likely Decline in Proposals Aug 27, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Sep 2, 2020

Based on the discussion above, this seems like a likely decline.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 16, 2020

No change in consensus, so declined.

@rsc rsc closed this Sep 16, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.