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: encoding/xml: support context on Marshaler/MarshalerAttr interfaces #64907

Open
awy opened this issue Dec 31, 2023 · 5 comments
Open
Labels
Milestone

Comments

@awy
Copy link

awy commented Dec 31, 2023

Proposal Details

To some extent this is very similar to encoding/xml: context-aware XML decoders and encoding/json supports Marshaler/Unmarshaler interfaces with a context.Context. It is different in that it is specific to XML encoding but in spirit it covers both the other two as well.

  1. Add func NewEncoderWithContext(ctx context.Context, w io.Writer) *Encoder
  2. Add func (*Encoder) Context() context.Context
  3. Add type ContextMarshalerAttr interface containing MarshalXMLAttrWithContext(ctx context.Context, name Name) (Attr, error)

Use cases

(To paraphrase @as) An HTTP handler in a go program marshals an XML document in response to an HTTP request. To give two examples:

  1. the XML document contains some attributes that are localized (i18n);
  2. some attributes need to be masked either entirely or partially, due to some per-user data masking policy.

The context carries the i18n accepted language and the user information from which data masking policy is derived.

The response may be pre-prepared so that it may be marsahled for responses to different requests requiring different localization.

Example use

type TranslatableString string

func (ts *TranslatableString) MarshalXML(e *xml.Encoder, start xml.StartElement) error {
	var v string
	if ts != nil {
		v = i18n.GetString(e.Context(), *ts)
	}
	return e.EncodeElement(v, start)
}

func (ts *TranslatableString) MarshalXMLAttrWithContext(ctx context.Context, name xml.Name) (Attr, error) {
	var v string
	if ts != nil {
		v = i18n.GetString(ctx, *ts)
	}
	return xml.Attr{Name: name, Value: v}, nil
}

Previous rebuttals

@rsc commented on both the previous referenced proposals:

Contexts are not for per-operation options. It's for timeouts and more global things like authorization credentials, not fine-grained per-call options. ...

However https://go.dev/blog/context specifically documents use of contexts in this manner with regard to extracting a user IP address from a request and associating it with a Context so that it may be used later in the request processing by a different package.

@awy awy added the Proposal label Dec 31, 2023
@gopherbot gopherbot added this to the Proposal milestone Dec 31, 2023
@awy
Copy link
Author

awy commented Dec 31, 2023

cc: @sebastien-rosset @as

@awy
Copy link
Author

awy commented Dec 31, 2023

Possible implementation

diff --git a/encoding/xml/marshal.go b/encoding/xml/marshal.go
index ae39846..b25ce3f 100644
--- a/encoding/xml/marshal.go
+++ b/encoding/xml/marshal.go
@@ -7,6 +7,7 @@ package xml
 import (
 	"bufio"
 	"bytes"
+	"context"
 	"encoding"
 	"errors"
 	"fmt"
@@ -105,6 +106,8 @@ func Marshal(v any) ([]byte, error) {
 // to generate the XML output one token at a time.
 // The sequence of encoded tokens must make up zero or more valid
 // XML elements.
+// An implementation may access any Context passed to NewEncoderWithContext
+// via e.Context()
 type Marshaler interface {
 	MarshalXML(e *Encoder, start StartElement) error
 }
@@ -124,6 +127,13 @@ type MarshalerAttr interface {
 	MarshalXMLAttr(name Name) (Attr, error)
 }
 
+// ContextMarshalerAttr is the interface implemented by objects that can marshal
+// themselves into valid XML attributes and with reference to the Context provided
+// to NewEncoderWithContext.
+type ContextMarshalerAttr interface {
+	MarshalXMLAttrWithContext(ctx context.Context, name Name) (Attr, error)
+}
+
 // MarshalIndent works like Marshal, but each XML element begins on a new
 // indented line that starts with prefix and is followed by one or more
 // copies of indent according to the nesting depth.
@@ -142,16 +152,29 @@ func MarshalIndent(v any, prefix, indent string) ([]byte, error) {
 
 // An Encoder writes XML data to an output stream.
 type Encoder struct {
-	p printer
+	p   printer
+	ctx context.Context
 }
 
 // NewEncoder returns a new encoder that writes to w.
 func NewEncoder(w io.Writer) *Encoder {
-	e := &Encoder{printer{w: bufio.NewWriter(w)}}
+	//lint:ignore SA1012 allow nil context when not created with NewEncoderWithContext
+	return NewEncoderWithContext(nil, w)
+}
+
+// NewEncoderWithContext returns a new encoder that writes to w
+// and has an associated Context.
+func NewEncoderWithContext(ctx context.Context, w io.Writer) *Encoder {
+	e := &Encoder{
+		p:   printer{w: bufio.NewWriter(w)},
+		ctx: ctx,
+	}
 	e.p.encoder = e
 	return e
 }
 
+func (enc *Encoder) Context() context.Context { return enc.ctx }
+
 // Indent sets the encoder to generate XML in which each element
 // begins on a new indented line that starts with prefix and is followed by
 // one or more copies of indent according to the nesting depth.
@@ -415,9 +438,10 @@ func (p *printer) popPrefix() {
 }
 
 var (
-	marshalerType     = reflect.TypeOf((*Marshaler)(nil)).Elem()
-	marshalerAttrType = reflect.TypeOf((*MarshalerAttr)(nil)).Elem()
-	textMarshalerType = reflect.TypeOf((*encoding.TextMarshaler)(nil)).Elem()
+	marshalerType            = reflect.TypeOf((*Marshaler)(nil)).Elem()
+	marshalerAttrType        = reflect.TypeOf((*MarshalerAttr)(nil)).Elem()
+	contextMarshalerAttrType = reflect.TypeOf((*ContextMarshalerAttr)(nil)).Elem()
+	textMarshalerType        = reflect.TypeOf((*encoding.TextMarshaler)(nil)).Elem()
 )
 
 // marshalValue writes one or more XML elements representing val.
@@ -578,6 +602,17 @@ func (p *printer) marshalValue(val reflect.Value, finfo *fieldInfo, startTemplat
 
 // marshalAttr marshals an attribute with the given name and value, adding to start.Attr.
 func (p *printer) marshalAttr(start *StartElement, name Name, val reflect.Value) error {
+	if val.CanInterface() && val.Type().Implements(contextMarshalerAttrType) {
+		attr, err := val.Interface().(ContextMarshalerAttr).MarshalXMLAttrWithContext(p.encoder.Context(), name)
+		if err != nil {
+			return err
+		}
+		if attr.Name.Local != "" {
+			start.Attr = append(start.Attr, attr)
+		}
+		return nil
+	}
+
 	if val.CanInterface() && val.Type().Implements(marshalerAttrType) {
 		attr, err := val.Interface().(MarshalerAttr).MarshalXMLAttr(name)
 		if err != nil {
@@ -591,6 +626,18 @@ func (p *printer) marshalAttr(start *StartElement, name Name, val reflect.Value)
 
 	if val.CanAddr() {
 		pv := val.Addr()
+
+		if pv.CanInterface() && pv.Type().Implements(contextMarshalerAttrType) {
+			attr, err := pv.Interface().(ContextMarshalerAttr).MarshalXMLAttrWithContext(p.encoder.Context(), name)
+			if err != nil {
+				return err
+			}
+			if attr.Name.Local != "" {
+				start.Attr = append(start.Attr, attr)
+			}
+			return nil
+		}
+
 		if pv.CanInterface() && pv.Type().Implements(marshalerAttrType) {
 			attr, err := pv.Interface().(MarshalerAttr).MarshalXMLAttr(name)
 			if err != nil {

@seankhliao seankhliao changed the title proposal: encoding/xml supports context on Marshaler/MarshalerAttr interfaces proposal: encoding/xml: support context on Marshaler/MarshalerAttr interfaces Jan 1, 2024
@apparentlymart
Copy link

The way I've typically thought about use of context.Context values is as a way to model cross-cutting concerns, particularly when those concerns potentially need to pass through an API that isn't (and has no need to be) aware of them in order to reach another API that does share the concern.

Trying to apply that idea to your proposal, I could see an argument that it might be useful to model "human languages preferred by the end-user" as a cross-cutting concern in that sense, but I would find it surprising for something like the XML library to directly rely on that, as opposed to the calling application making that decision and then communicating that to the XML library through a new explicit API, rather than by making the XML library accept a context and pull values out itself.

I think if it seems like treating this as a cross-cutting concern is the right idea (contrary to my argument above) then it would be a good idea to identify what other libraries might make use of this new convention, and make sure the design is sufficient to serve at least a significant number of those use-cases. It seems tricky to justify considering the possibility of passing a language preference between functions as a cross cutting concern if in practice only one package would make use of it. 🤔

@apparentlymart
Copy link

apparentlymart commented Jan 2, 2024

I pondered this some more after my first response and found a different way to think about it that caused me to feel more favorable about it:

This proposal is not really about passing the preferred languages to the XML library, but is instead about passing that information through the XML library to some marshalling code that lives inside the calling application.

In that case, the cross-cutting concern is the ability for an application to pass arbitrary data to its own unmarshalling implementations, regardless of what that data is. I can definitely identify with that need, and have used contexts to achieve similar things in the past myself, but it does always feel a little odd whenever I end up adding a context argument to a function that isn't doing interruptible I/O. I don't know of any better mechanisms for passing cross-cutting concerns in this way, though.

@awy
Copy link
Author

awy commented Jan 3, 2024

This proposal is not really about passing the preferred languages to the XML library, but is instead about passing that information through the XML library to some marshalling code that lives inside the calling application.

In that case, the cross-cutting concern is the ability for an application to pass arbitrary data to its own unmarshalling implementations, regardless of what that data is.

Yes exactly. The specific example that I gave was for user-preferred language (which is the driving factor for me) but I also mentioned data-filtering based on other request properties. I can imagine other use-cases.

Although Context out-of-the-box provides support just for cancellation (including with timeout), the very fact that it allows application-specific values to be attached seems to be a strong indication that this sort of use was explicitly foreseen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

3 participants