From bf061bede4b0280654c25b19fc3cc34e25d13cf0 Mon Sep 17 00:00:00 2001 From: Naina Raisinghani Date: Mon, 27 Apr 2020 11:29:31 -0700 Subject: [PATCH 01/26] Create CODE_OF_CONDUCT.md (#410) --- CODE_OF_CONDUCT.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 CODE_OF_CONDUCT.md diff --git a/CODE_OF_CONDUCT.md b/CODE_OF_CONDUCT.md new file mode 100644 index 000000000..b74de28e0 --- /dev/null +++ b/CODE_OF_CONDUCT.md @@ -0,0 +1 @@ +See https://github.com/ampproject/meta/blob/master/CODE_OF_CONDUCT.md From 47b50005d6cb5de1fd8023c36b44a681ed37ddc0 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Fri, 10 Jul 2020 00:59:24 -0700 Subject: [PATCH 02/26] initial creation of transformer --- transformer/transformers/scriptsrcrewrite.go | 62 +++++++++++++++++++ .../transformers/scriptsrcrewrite_test.go | 0 2 files changed, 62 insertions(+) create mode 100644 transformer/transformers/scriptsrcrewrite.go create mode 100644 transformer/transformers/scriptsrcrewrite_test.go diff --git a/transformer/transformers/scriptsrcrewrite.go b/transformer/transformers/scriptsrcrewrite.go new file mode 100644 index 000000000..8fe6e76ed --- /dev/null +++ b/transformer/transformers/scriptsrcrewrite.go @@ -0,0 +1,62 @@ +package transformers + +import ( + "regexp" + "strings" + + "github.com/ampproject/amppackager/transformer/internal/amphtml" + "github.com/ampproject/amppackager/transformer/internal/htmlnode" + "golang.org/x/net/html/atom" + "golang.org/x/net/html" +) + +type rewritable interface { + // Rewrite the URLs within + rewrite(string, *url.URL, string, nodeMap) +} + +type urlRewriteContext []rewritable + +// textNodeContext qualifies a text node, whose entire data value has URLs embedded +// within. This struct keeps track of where the URLs occur in the body of the text. +type textNodeContext struct { + node *html.Node + offsets []amphtml.SubresourceOffset +} + +// This matches strings that end with ".js". +var jsRE = func() *regexp.Regexp { + r = regexp.MustCompile("^(.)*\.js") + return r +}() + +// This matches strings that end with ".sxg.js". +var sxgJsRE = func() *regexp.Regexp { + r = regexp.MustCompile("^(.)*\.sxg\.js") + return r +}() + +// ScriptSrcRewrite rewrites the value of src in ", + Expected: "", + }, + } + + for _, tc := range tcs { + inputDoc, err := html.Parse(strings.NewReader(tc.Input)) + if err != nil { + t.Errorf("%s: html.Parse failed %q", tc.Input, err) + continue + } + inputDOM, err := amphtml.NewDOM(inputDoc) + if err != nil { + t.Errorf("%s\namphtml.NewDOM for %s failed %q", tc.Desc, tc.Input, err) + continue + } + transformers.AMPRuntimeJS(&transformers.Context{DOM: inputDOM}) + var input strings.Builder + if err := html.Render(&input, inputDoc); err != nil { + t.Errorf("%s: html.Render failed %q", tc.Input, err) + continue + } + + expectedDoc, err := html.Parse(strings.NewReader(tc.Expected)) + if err != nil { + t.Errorf("%s: html.Parse failed %q", tc.Expected, err) + continue + } + var expected strings.Builder + err = html.Render(&expected, expectedDoc) + if err != nil { + t.Errorf("%s: html.Render failed %q", tc.Expected, err) + continue + } + if input.String() != expected.String() { + t.Errorf("%s: Transform=\n%q\nwant=\n%q", tc.Desc, &input, &expected) + } + } +} \ No newline at end of file From dff39de9a7683c78bb4a98f76574ce1dd653ffbc Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Fri, 10 Jul 2020 18:38:38 -0700 Subject: [PATCH 04/26] transformer rename --- transformer/transformers/{scriptsrcrewrite.go => ampruntimejs.go} | 0 .../{scriptsrcrewrite_test.go => ampruntimejs_test.go} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename transformer/transformers/{scriptsrcrewrite.go => ampruntimejs.go} (100%) rename transformer/transformers/{scriptsrcrewrite_test.go => ampruntimejs_test.go} (100%) diff --git a/transformer/transformers/scriptsrcrewrite.go b/transformer/transformers/ampruntimejs.go similarity index 100% rename from transformer/transformers/scriptsrcrewrite.go rename to transformer/transformers/ampruntimejs.go diff --git a/transformer/transformers/scriptsrcrewrite_test.go b/transformer/transformers/ampruntimejs_test.go similarity index 100% rename from transformer/transformers/scriptsrcrewrite_test.go rename to transformer/transformers/ampruntimejs_test.go From b099fd13217ca2c2c85713094e6349acf2988fa7 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Mon, 10 Aug 2020 22:21:00 -0700 Subject: [PATCH 05/26] .sxg.js to &f=sxg --- transformer/transformers/ampruntimejs.go | 2 +- transformer/transformers/ampruntimejs_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index 400ed50ad..41656413c 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -19,7 +19,7 @@ func AMPRuntimeJS(e *Context) error { } if n.DataAtom == atom.Script { if src, ok = htmlnode.FindAttribute(n, "", "src"); ok && strings.hasPrefix(src.Val, amphtml.AMPCacheRootURL) && strings.HasSuffix(src.Val, ".js"){ - src.Val = strings.TrimSuffix(src.Val, ".js") + ".sxg.js" + src.Val = strings.TrimSuffix(src.Val, ".js") + ".js&f=sxg" } } else { continue diff --git a/transformer/transformers/ampruntimejs_test.go b/transformer/transformers/ampruntimejs_test.go index 7c598f7db..eaa3a53c0 100644 --- a/transformer/transformers/ampruntimejs_test.go +++ b/transformer/transformers/ampruntimejs_test.go @@ -30,7 +30,7 @@ func TestStripJS(t *testing.T) { { Desc: "transformation", Input: "", - Expected: "", + Expected: "", }, } From 81b97fbb8205fd9c3bdb812009c1454b2c8d838d Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Mon, 10 Aug 2020 22:46:23 -0700 Subject: [PATCH 06/26] use of go url api --- transformer/transformers/ampruntimejs.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index 41656413c..26db70547 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -8,6 +8,7 @@ import ( "github.com/ampproject/amppackager/transformer/internal/htmlnode" "golang.org/x/net/html/atom" "golang.org/x/net/html" + "golang.org/x/net/html/url" ) // AMPRuntimeJS rewrites the value of src in script nodes, where applicable. @@ -18,8 +19,15 @@ func AMPRuntimeJS(e *Context) error { continue } if n.DataAtom == atom.Script { - if src, ok = htmlnode.FindAttribute(n, "", "src"); ok && strings.hasPrefix(src.Val, amphtml.AMPCacheRootURL) && strings.HasSuffix(src.Val, ".js"){ - src.Val = strings.TrimSuffix(src.Val, ".js") + ".js&f=sxg" + if src, ok = htmlnode.FindAttribute(n, "", "src"); ok && strings.hasPrefix(src.Val, amphtml.AMPCacheRootURL){ + u, _ = url.Parse(src.Val) + query, _ = url.ParseQuery(u.RawQuery) + path = u.Path + if strings.HasSuffix(path, ".js"){ + query.Add("f", "sxg") + u.RawQuery = query.Encode() + src.Val = u.String() + } } } else { continue From 81dbcc802fb690be68200ad3c19ceef967144ae0 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Mon, 10 Aug 2020 23:41:01 -0700 Subject: [PATCH 07/26] import error --- transformer/transformers/ampruntimejs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index 26db70547..5a8018f85 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -3,12 +3,12 @@ package transformers import ( "regexp" "strings" + "net/url" "github.com/ampproject/amppackager/transformer/internal/amphtml" "github.com/ampproject/amppackager/transformer/internal/htmlnode" "golang.org/x/net/html/atom" "golang.org/x/net/html" - "golang.org/x/net/html/url" ) // AMPRuntimeJS rewrites the value of src in script nodes, where applicable. From c69061c17a2ca957cc79e49e52454cab41affb99 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Wed, 12 Aug 2020 16:25:56 -0700 Subject: [PATCH 08/26] compiler errors --- transformer/transformers/ampruntimejs.go | 8 ++++---- transformer/transformers/ampruntimejs_test.go | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index 400ed50ad..51f7a0cf2 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -1,13 +1,12 @@ package transformers import ( - "regexp" "strings" "github.com/ampproject/amppackager/transformer/internal/amphtml" "github.com/ampproject/amppackager/transformer/internal/htmlnode" - "golang.org/x/net/html/atom" "golang.org/x/net/html" + "golang.org/x/net/html/atom" ) // AMPRuntimeJS rewrites the value of src in script nodes, where applicable. @@ -18,7 +17,8 @@ func AMPRuntimeJS(e *Context) error { continue } if n.DataAtom == atom.Script { - if src, ok = htmlnode.FindAttribute(n, "", "src"); ok && strings.hasPrefix(src.Val, amphtml.AMPCacheRootURL) && strings.HasSuffix(src.Val, ".js"){ + src, ok := htmlnode.FindAttribute(n, "", "src") + if ok && strings.HasPrefix(src.Val, amphtml.AMPCacheRootURL) && strings.HasSuffix(src.Val, ".js") { src.Val = strings.TrimSuffix(src.Val, ".js") + ".sxg.js" } } else { @@ -26,4 +26,4 @@ func AMPRuntimeJS(e *Context) error { } } return nil -} \ No newline at end of file +} diff --git a/transformer/transformers/ampruntimejs_test.go b/transformer/transformers/ampruntimejs_test.go index 7c598f7db..2bb6ec66c 100644 --- a/transformer/transformers/ampruntimejs_test.go +++ b/transformer/transformers/ampruntimejs_test.go @@ -10,7 +10,7 @@ import ( "golang.org/x/net/html" ) -func TestStripJS(t *testing.T) { +func TestAmpRuntimeJS(t *testing.T) { tcs := []tt.TestCase{ { Desc: "no script node", @@ -67,4 +67,4 @@ func TestStripJS(t *testing.T) { t.Errorf("%s: Transform=\n%q\nwant=\n%q", tc.Desc, &input, &expected) } } -} \ No newline at end of file +} From 8a9d719ea89807f5085796570bbd66c572b4e951 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Wed, 12 Aug 2020 16:33:51 -0700 Subject: [PATCH 09/26] compiler errors w/ url --- transformer/transformers/ampruntimejs.go | 18 +++++++++--------- transformer/transformers/ampruntimejs_test.go | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index 5a8018f85..788433fb1 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -1,14 +1,13 @@ package transformers import ( - "regexp" - "strings" "net/url" + "strings" "github.com/ampproject/amppackager/transformer/internal/amphtml" "github.com/ampproject/amppackager/transformer/internal/htmlnode" - "golang.org/x/net/html/atom" "golang.org/x/net/html" + "golang.org/x/net/html/atom" ) // AMPRuntimeJS rewrites the value of src in script nodes, where applicable. @@ -19,11 +18,12 @@ func AMPRuntimeJS(e *Context) error { continue } if n.DataAtom == atom.Script { - if src, ok = htmlnode.FindAttribute(n, "", "src"); ok && strings.hasPrefix(src.Val, amphtml.AMPCacheRootURL){ - u, _ = url.Parse(src.Val) - query, _ = url.ParseQuery(u.RawQuery) - path = u.Path - if strings.HasSuffix(path, ".js"){ + src, ok := htmlnode.FindAttribute(n, "", "src") + if ok && strings.HasPrefix(src.Val, amphtml.AMPCacheRootURL) { + u, _ := url.Parse(src.Val) + query, _ := url.ParseQuery(u.RawQuery) + path := u.Path + if strings.HasSuffix(path, ".js") { query.Add("f", "sxg") u.RawQuery = query.Encode() src.Val = u.String() @@ -34,4 +34,4 @@ func AMPRuntimeJS(e *Context) error { } } return nil -} \ No newline at end of file +} diff --git a/transformer/transformers/ampruntimejs_test.go b/transformer/transformers/ampruntimejs_test.go index eaa3a53c0..e3ad9af71 100644 --- a/transformer/transformers/ampruntimejs_test.go +++ b/transformer/transformers/ampruntimejs_test.go @@ -10,7 +10,7 @@ import ( "golang.org/x/net/html" ) -func TestStripJS(t *testing.T) { +func TestAmpRuntimeJS(t *testing.T) { tcs := []tt.TestCase{ { Desc: "no script node", @@ -67,4 +67,4 @@ func TestStripJS(t *testing.T) { t.Errorf("%s: Transform=\n%q\nwant=\n%q", tc.Desc, &input, &expected) } } -} \ No newline at end of file +} From 749ef8595a70d7a21d65391541b19738815d49fc Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Wed, 12 Aug 2020 16:56:37 -0700 Subject: [PATCH 10/26] correction in test --- transformer/transformers/ampruntimejs_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/transformer/transformers/ampruntimejs_test.go b/transformer/transformers/ampruntimejs_test.go index e3ad9af71..3c84185da 100644 --- a/transformer/transformers/ampruntimejs_test.go +++ b/transformer/transformers/ampruntimejs_test.go @@ -30,7 +30,7 @@ func TestAmpRuntimeJS(t *testing.T) { { Desc: "transformation", Input: "", - Expected: "", + Expected: "", }, } From 2c4570183e4384455dbcdbc213369f1b0f5b2a62 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Wed, 12 Aug 2020 17:02:19 -0700 Subject: [PATCH 11/26] merge conflict --- transformer/transformers/ampruntimejs.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index 51f7a0cf2..788433fb1 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -1,6 +1,7 @@ package transformers import ( + "net/url" "strings" "github.com/ampproject/amppackager/transformer/internal/amphtml" @@ -18,8 +19,15 @@ func AMPRuntimeJS(e *Context) error { } if n.DataAtom == atom.Script { src, ok := htmlnode.FindAttribute(n, "", "src") - if ok && strings.HasPrefix(src.Val, amphtml.AMPCacheRootURL) && strings.HasSuffix(src.Val, ".js") { - src.Val = strings.TrimSuffix(src.Val, ".js") + ".sxg.js" + if ok && strings.HasPrefix(src.Val, amphtml.AMPCacheRootURL) { + u, _ := url.Parse(src.Val) + query, _ := url.ParseQuery(u.RawQuery) + path := u.Path + if strings.HasSuffix(path, ".js") { + query.Add("f", "sxg") + u.RawQuery = query.Encode() + src.Val = u.String() + } } } else { continue From 126558929a854c494611a736eec587b1d46e2653 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Wed, 12 Aug 2020 17:03:02 -0700 Subject: [PATCH 12/26] correction to test --- transformer/transformers/ampruntimejs_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/transformer/transformers/ampruntimejs_test.go b/transformer/transformers/ampruntimejs_test.go index 2bb6ec66c..3c84185da 100644 --- a/transformer/transformers/ampruntimejs_test.go +++ b/transformer/transformers/ampruntimejs_test.go @@ -30,7 +30,7 @@ func TestAmpRuntimeJS(t *testing.T) { { Desc: "transformation", Input: "", - Expected: "", + Expected: "", }, } From addc2865f3eec4ce63a978f433defbbafd677891 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Wed, 12 Aug 2020 17:12:50 -0700 Subject: [PATCH 13/26] 14 transformers present --- transformer/transformer_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/transformer/transformer_test.go b/transformer/transformer_test.go index 32ff55527..4c90cd7ab 100644 --- a/transformer/transformer_test.go +++ b/transformer/transformer_test.go @@ -6,9 +6,9 @@ import ( "strings" "testing" - "github.com/golang/protobuf/proto" rpb "github.com/ampproject/amppackager/transformer/request" "github.com/ampproject/amppackager/transformer/transformers" + "github.com/golang/protobuf/proto" "github.com/google/go-cmp/cmp" ) @@ -27,7 +27,7 @@ func TestProcess(t *testing.T) { config rpb.Request_TransformersConfig expectedLen int }{ - {rpb.Request_DEFAULT, 13}, + {rpb.Request_DEFAULT, 14}, {rpb.Request_NONE, 0}, {rpb.Request_VALIDATION, 1}, {rpb.Request_CUSTOM, 0}, From 0a355d49b65b4ef67e0d9e54fc83de4a5bd7053d Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Wed, 12 Aug 2020 17:49:16 -0700 Subject: [PATCH 14/26] minor semantic/syntactical changes --- transformer/transformer.go | 11 +++++------ transformer/transformers/ampruntimejs.go | 6 ++---- transformer/transformers/ampruntimejs_test.go | 8 ++++---- 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/transformer/transformer.go b/transformer/transformer.go index 969757123..a07be5b12 100644 --- a/transformer/transformer.go +++ b/transformer/transformer.go @@ -30,8 +30,8 @@ import ( rpb "github.com/ampproject/amppackager/transformer/request" "github.com/ampproject/amppackager/transformer/transformers" "github.com/pkg/errors" - "golang.org/x/net/html/atom" "golang.org/x/net/html" + "golang.org/x/net/html/atom" ) // Transformer functions must be added here in order to be passed in from @@ -44,7 +44,7 @@ var transformerFunctionMap = map[string]func(*transformers.Context) error{ "absoluteurl": transformers.AbsoluteURL, "ampboilerplate": transformers.AMPBoilerplate, "ampruntimecss": transformers.AMPRuntimeCSS, - "ampruntimejs": transformers.AMPRuntimeJS, + "ampruntimejs": transformers.AMPRuntimeJS, "linktag": transformers.LinkTag, "nodecleanup": transformers.NodeCleanup, "preloadimage": transformers.PreloadImage, @@ -92,7 +92,6 @@ var configMap = map[rpb.Request_TransformersConfig][]func(*transformers.Context) // from an unnecessary number of fetches. const maxPreloads = 20 - // Override for tests. var runTransformers = func(c *transformers.Context, fns []func(*transformers.Context) error) error { // Invoke the configured transformers @@ -226,11 +225,11 @@ func extractPreloads(dom *amphtml.DOM) []*rpb.Metadata_Preload { // amp-script without an explicit max-age. This is 1 day, to parallel the // security precautions put in place around service workers: // https://dev.chromium.org/Home/chromium-security/security-faq/service-worker-security-faq#TOC-Do-Service-Workers-live-forever- -const defaultMaxAgeSeconds int32 = 86400 // number of seconds in a day +const defaultMaxAgeSeconds int32 = 86400 // number of seconds in a day // maxMaxAgeSeconds is the max duration of an SXG, per // https://wicg.github.io/webpackage/draft-yasskin-http-origin-signed-responses.html#signature-validity. -const maxMaxAgeSeconds int32 = 7*86400 +const maxMaxAgeSeconds int32 = 7 * 86400 // computeMaxAgeSeconds returns the suggested max-age based on the presence of // any inline tags on the page; callers should min() the return @@ -319,7 +318,7 @@ func Process(r *rpb.Request) (string, *rpb.Metadata, error) { return "", nil, err } metadata := rpb.Metadata{ - Preloads: extractPreloads(context.DOM), + Preloads: extractPreloads(context.DOM), MaxAgeSecs: computeMaxAgeSeconds(context.DOM), } return o.String(), &metadata, nil diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index 788433fb1..e5d6a7a30 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -11,7 +11,7 @@ import ( ) // AMPRuntimeJS rewrites the value of src in script nodes, where applicable. -// If the value is of the form "*.js", replace it with "*.sxg.js". +// If the value is of the form "*.js", replace it with "*.js?f=sxg". func AMPRuntimeJS(e *Context) error { for n := e.DOM.RootNode; n != nil; n = htmlnode.Next(n) { if n.Type != html.ElementNode { @@ -24,13 +24,11 @@ func AMPRuntimeJS(e *Context) error { query, _ := url.ParseQuery(u.RawQuery) path := u.Path if strings.HasSuffix(path, ".js") { - query.Add("f", "sxg") + query.Set("f", "sxg") u.RawQuery = query.Encode() src.Val = u.String() } } - } else { - continue } } return nil diff --git a/transformer/transformers/ampruntimejs_test.go b/transformer/transformers/ampruntimejs_test.go index 3c84185da..859e582ed 100644 --- a/transformer/transformers/ampruntimejs_test.go +++ b/transformer/transformers/ampruntimejs_test.go @@ -24,13 +24,13 @@ func TestAmpRuntimeJS(t *testing.T) { }, { Desc: "no suffix", - Input: "", - Expected: "", + Input: ``, + Expected: ``, }, } From 95cca8838f0d33d3f8e7e756f451411444088d00 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Wed, 12 Aug 2020 17:53:11 -0700 Subject: [PATCH 15/26] headnode --- transformer/transformers/ampruntimejs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index e5d6a7a30..071b7e3e1 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -13,7 +13,7 @@ import ( // AMPRuntimeJS rewrites the value of src in script nodes, where applicable. // If the value is of the form "*.js", replace it with "*.js?f=sxg". func AMPRuntimeJS(e *Context) error { - for n := e.DOM.RootNode; n != nil; n = htmlnode.Next(n) { + for n := e.DOM.HeadNode; n != nil; n = htmlnode.Next(n) { if n.Type != html.ElementNode { continue } From fc36393f48fd2623257476e635b030571b77b0f9 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Wed, 12 Aug 2020 17:55:15 -0700 Subject: [PATCH 16/26] format --- transformer/transformers/ampruntimejs.go | 70 ++++----- transformer/transformers/ampruntimejs_test.go | 140 +++++++++--------- 2 files changed, 105 insertions(+), 105 deletions(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index 071b7e3e1..354db1e47 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -1,35 +1,35 @@ -package transformers - -import ( - "net/url" - "strings" - - "github.com/ampproject/amppackager/transformer/internal/amphtml" - "github.com/ampproject/amppackager/transformer/internal/htmlnode" - "golang.org/x/net/html" - "golang.org/x/net/html/atom" -) - -// AMPRuntimeJS rewrites the value of src in script nodes, where applicable. -// If the value is of the form "*.js", replace it with "*.js?f=sxg". -func AMPRuntimeJS(e *Context) error { - for n := e.DOM.HeadNode; n != nil; n = htmlnode.Next(n) { - if n.Type != html.ElementNode { - continue - } - if n.DataAtom == atom.Script { - src, ok := htmlnode.FindAttribute(n, "", "src") - if ok && strings.HasPrefix(src.Val, amphtml.AMPCacheRootURL) { - u, _ := url.Parse(src.Val) - query, _ := url.ParseQuery(u.RawQuery) - path := u.Path - if strings.HasSuffix(path, ".js") { - query.Set("f", "sxg") - u.RawQuery = query.Encode() - src.Val = u.String() - } - } - } - } - return nil -} +package transformers + +import ( + "net/url" + "strings" + + "github.com/ampproject/amppackager/transformer/internal/amphtml" + "github.com/ampproject/amppackager/transformer/internal/htmlnode" + "golang.org/x/net/html" + "golang.org/x/net/html/atom" +) + +// AMPRuntimeJS rewrites the value of src in script nodes, where applicable. +// If the value is of the form "*.js", replace it with "*.js?f=sxg". +func AMPRuntimeJS(e *Context) error { + for n := e.DOM.HeadNode; n != nil; n = htmlnode.Next(n) { + if n.Type != html.ElementNode { + continue + } + if n.DataAtom == atom.Script { + src, ok := htmlnode.FindAttribute(n, "", "src") + if ok && strings.HasPrefix(src.Val, amphtml.AMPCacheRootURL) { + u, _ := url.Parse(src.Val) + query, _ := url.ParseQuery(u.RawQuery) + path := u.Path + if strings.HasSuffix(path, ".js") { + query.Set("f", "sxg") + u.RawQuery = query.Encode() + src.Val = u.String() + } + } + } + } + return nil +} diff --git a/transformer/transformers/ampruntimejs_test.go b/transformer/transformers/ampruntimejs_test.go index 859e582ed..c429f64dc 100644 --- a/transformer/transformers/ampruntimejs_test.go +++ b/transformer/transformers/ampruntimejs_test.go @@ -1,70 +1,70 @@ -package transformers_test - -import ( - "strings" - "testing" - - "github.com/ampproject/amppackager/transformer/internal/amphtml" - tt "github.com/ampproject/amppackager/transformer/internal/testing" - "github.com/ampproject/amppackager/transformer/transformers" - "golang.org/x/net/html" -) - -func TestAmpRuntimeJS(t *testing.T) { - tcs := []tt.TestCase{ - { - Desc: "no script node", - Input: "", - Expected: "", - }, - { - Desc: "no prefix", - Input: "`, - Expected: ``, - }, - } - - for _, tc := range tcs { - inputDoc, err := html.Parse(strings.NewReader(tc.Input)) - if err != nil { - t.Errorf("%s: html.Parse failed %q", tc.Input, err) - continue - } - inputDOM, err := amphtml.NewDOM(inputDoc) - if err != nil { - t.Errorf("%s\namphtml.NewDOM for %s failed %q", tc.Desc, tc.Input, err) - continue - } - transformers.AMPRuntimeJS(&transformers.Context{DOM: inputDOM}) - var input strings.Builder - if err := html.Render(&input, inputDoc); err != nil { - t.Errorf("%s: html.Render failed %q", tc.Input, err) - continue - } - - expectedDoc, err := html.Parse(strings.NewReader(tc.Expected)) - if err != nil { - t.Errorf("%s: html.Parse failed %q", tc.Expected, err) - continue - } - var expected strings.Builder - err = html.Render(&expected, expectedDoc) - if err != nil { - t.Errorf("%s: html.Render failed %q", tc.Expected, err) - continue - } - if input.String() != expected.String() { - t.Errorf("%s: Transform=\n%q\nwant=\n%q", tc.Desc, &input, &expected) - } - } -} +package transformers_test + +import ( + "strings" + "testing" + + "github.com/ampproject/amppackager/transformer/internal/amphtml" + tt "github.com/ampproject/amppackager/transformer/internal/testing" + "github.com/ampproject/amppackager/transformer/transformers" + "golang.org/x/net/html" +) + +func TestAmpRuntimeJS(t *testing.T) { + tcs := []tt.TestCase{ + { + Desc: "no script node", + Input: "", + Expected: "", + }, + { + Desc: "no prefix", + Input: "`, + Expected: ``, + }, + } + + for _, tc := range tcs { + inputDoc, err := html.Parse(strings.NewReader(tc.Input)) + if err != nil { + t.Errorf("%s: html.Parse failed %q", tc.Input, err) + continue + } + inputDOM, err := amphtml.NewDOM(inputDoc) + if err != nil { + t.Errorf("%s\namphtml.NewDOM for %s failed %q", tc.Desc, tc.Input, err) + continue + } + transformers.AMPRuntimeJS(&transformers.Context{DOM: inputDOM}) + var input strings.Builder + if err := html.Render(&input, inputDoc); err != nil { + t.Errorf("%s: html.Render failed %q", tc.Input, err) + continue + } + + expectedDoc, err := html.Parse(strings.NewReader(tc.Expected)) + if err != nil { + t.Errorf("%s: html.Parse failed %q", tc.Expected, err) + continue + } + var expected strings.Builder + err = html.Render(&expected, expectedDoc) + if err != nil { + t.Errorf("%s: html.Render failed %q", tc.Expected, err) + continue + } + if input.String() != expected.String() { + t.Errorf("%s: Transform=\n%q\nwant=\n%q", tc.Desc, &input, &expected) + } + } +} From 8bcdf4542c00c953688b4f40babcc27c63ed991d Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Wed, 12 Aug 2020 19:34:28 -0700 Subject: [PATCH 17/26] next sibling --- transformer/transformers/ampruntimejs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index 354db1e47..fa89e6372 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -13,7 +13,7 @@ import ( // AMPRuntimeJS rewrites the value of src in script nodes, where applicable. // If the value is of the form "*.js", replace it with "*.js?f=sxg". func AMPRuntimeJS(e *Context) error { - for n := e.DOM.HeadNode; n != nil; n = htmlnode.Next(n) { + for n := e.DOM.HeadNode; n != nil; n = n.NextSibling { if n.Type != html.ElementNode { continue } From 79c2296cb49fc4e2c84b219599e782f38a40c89b Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Wed, 12 Aug 2020 19:38:28 -0700 Subject: [PATCH 18/26] additional test --- transformer/transformers/ampruntimejs_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/transformer/transformers/ampruntimejs_test.go b/transformer/transformers/ampruntimejs_test.go index c429f64dc..2ea0625b2 100644 --- a/transformer/transformers/ampruntimejs_test.go +++ b/transformer/transformers/ampruntimejs_test.go @@ -32,6 +32,11 @@ func TestAmpRuntimeJS(t *testing.T) { Input: ``, Expected: ``, }, + { + Desc: "transform on two scripts", + Input: ``, + Expected: ``, + }, } for _, tc := range tcs { From e9295bf471130e139bd3eaa7b16661a23073dd77 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Wed, 12 Aug 2020 19:44:53 -0700 Subject: [PATCH 19/26] revert next --- transformer/transformers/ampruntimejs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index fa89e6372..354db1e47 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -13,7 +13,7 @@ import ( // AMPRuntimeJS rewrites the value of src in script nodes, where applicable. // If the value is of the form "*.js", replace it with "*.js?f=sxg". func AMPRuntimeJS(e *Context) error { - for n := e.DOM.HeadNode; n != nil; n = n.NextSibling { + for n := e.DOM.HeadNode; n != nil; n = htmlnode.Next(n) { if n.Type != html.ElementNode { continue } From ddb10fa4cc6219c0c6a779fe5734e80744bf843a Mon Sep 17 00:00:00 2001 From: "Derek X. R. Tse" Date: Wed, 12 Aug 2020 19:54:46 -0700 Subject: [PATCH 20/26] Rearranged import name? --- transformer/transformer_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/transformer/transformer_test.go b/transformer/transformer_test.go index 4c90cd7ab..0af4046fa 100644 --- a/transformer/transformer_test.go +++ b/transformer/transformer_test.go @@ -6,9 +6,9 @@ import ( "strings" "testing" + "github.com/golang/protobuf/proto" rpb "github.com/ampproject/amppackager/transformer/request" "github.com/ampproject/amppackager/transformer/transformers" - "github.com/golang/protobuf/proto" "github.com/google/go-cmp/cmp" ) From 74ecad220445964e5ad5ebb0e6eae035b9546a98 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Wed, 12 Aug 2020 21:41:15 -0700 Subject: [PATCH 21/26] wrap everything in head --- transformer/transformers/ampruntimejs.go | 2 +- transformer/transformers/ampruntimejs_test.go | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index 354db1e47..fa89e6372 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -13,7 +13,7 @@ import ( // AMPRuntimeJS rewrites the value of src in script nodes, where applicable. // If the value is of the form "*.js", replace it with "*.js?f=sxg". func AMPRuntimeJS(e *Context) error { - for n := e.DOM.HeadNode; n != nil; n = htmlnode.Next(n) { + for n := e.DOM.HeadNode; n != nil; n = n.NextSibling { if n.Type != html.ElementNode { continue } diff --git a/transformer/transformers/ampruntimejs_test.go b/transformer/transformers/ampruntimejs_test.go index 2ea0625b2..bbdb2801b 100644 --- a/transformer/transformers/ampruntimejs_test.go +++ b/transformer/transformers/ampruntimejs_test.go @@ -19,18 +19,18 @@ func TestAmpRuntimeJS(t *testing.T) { }, { Desc: "no prefix", - Input: "`, - Expected: ``, + Input: ``, + Expected: ``, }, { Desc: "transform on two scripts", From 82b4ad3a170c57683a52e4627763a4536e8b9fa8 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Thu, 13 Aug 2020 12:31:17 -0700 Subject: [PATCH 22/26] firstchild --- transformer/transformers/ampruntimejs.go | 2 +- transformer/transformers/ampruntimejs_test.go | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index fa89e6372..d4db3bee6 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -13,7 +13,7 @@ import ( // AMPRuntimeJS rewrites the value of src in script nodes, where applicable. // If the value is of the form "*.js", replace it with "*.js?f=sxg". func AMPRuntimeJS(e *Context) error { - for n := e.DOM.HeadNode; n != nil; n = n.NextSibling { + for n := e.DOM.HeadNode.FirstChild; n != nil; n = n.NextSibling { if n.Type != html.ElementNode { continue } diff --git a/transformer/transformers/ampruntimejs_test.go b/transformer/transformers/ampruntimejs_test.go index bbdb2801b..c56a2fdb9 100644 --- a/transformer/transformers/ampruntimejs_test.go +++ b/transformer/transformers/ampruntimejs_test.go @@ -37,6 +37,11 @@ func TestAmpRuntimeJS(t *testing.T) { Input: ``, Expected: ``, }, + { + Desc: "skip one, transform one", + Input: ``, + Expected: ``, + }, } for _, tc := range tcs { From dc4c310f94e09bc26d8f59d20b39901067ed5f4d Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Sat, 15 Aug 2020 00:23:50 -0700 Subject: [PATCH 23/26] various suggestions --- transformer/transformers/ampruntimejs.go | 10 +++++----- transformer/transformers/ampruntimejs_test.go | 20 +++++++++++++++++++ 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index d4db3bee6..be0a262a5 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -14,13 +14,13 @@ import ( // If the value is of the form "*.js", replace it with "*.js?f=sxg". func AMPRuntimeJS(e *Context) error { for n := e.DOM.HeadNode.FirstChild; n != nil; n = n.NextSibling { - if n.Type != html.ElementNode { - continue - } - if n.DataAtom == atom.Script { + if n.Type == html.ElementNode && n.DataAtom == atom.Script { src, ok := htmlnode.FindAttribute(n, "", "src") if ok && strings.HasPrefix(src.Val, amphtml.AMPCacheRootURL) { - u, _ := url.Parse(src.Val) + u, err := url.Parse(src.Val) + if err != nil { + continue + } query, _ := url.ParseQuery(u.RawQuery) path := u.Path if strings.HasSuffix(path, ".js") { diff --git a/transformer/transformers/ampruntimejs_test.go b/transformer/transformers/ampruntimejs_test.go index c56a2fdb9..7a7ad0592 100644 --- a/transformer/transformers/ampruntimejs_test.go +++ b/transformer/transformers/ampruntimejs_test.go @@ -42,6 +42,26 @@ func TestAmpRuntimeJS(t *testing.T) { Input: ``, Expected: ``, }, + { + Desc: "additional params exist", + Input: ``, + Expected: ``, + }, + { + Desc: "existing f param", + Input: ``, + Expected: ``, + }, + { + Desc: "parsing bug", + Input: ``, + Expected: ``, + }, + { + Desc: "parsing bug", + Input: ``, + Expected: ``, + } } for _, tc := range tcs { From 7eb243867359818a66829db73b6d613442afbc98 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Sat, 15 Aug 2020 00:35:28 -0700 Subject: [PATCH 24/26] rename test --- transformer/transformers/ampruntimejs_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/transformer/transformers/ampruntimejs_test.go b/transformer/transformers/ampruntimejs_test.go index 7a7ad0592..afffc2d1e 100644 --- a/transformer/transformers/ampruntimejs_test.go +++ b/transformer/transformers/ampruntimejs_test.go @@ -53,15 +53,15 @@ func TestAmpRuntimeJS(t *testing.T) { Expected: ``, }, { - Desc: "parsing bug", + Desc: "url escape parsing bug in query param", Input: ``, Expected: ``, }, { - Desc: "parsing bug", + Desc: "url escape parsing bug", Input: ``, Expected: ``, - } + }, } for _, tc := range tcs { From adcfd26a2a53b49534b9cbebff6138b847e8b287 Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Sat, 15 Aug 2020 00:49:33 -0700 Subject: [PATCH 25/26] more error --- transformer/transformers/ampruntimejs.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/transformer/transformers/ampruntimejs.go b/transformer/transformers/ampruntimejs.go index be0a262a5..4046c244e 100644 --- a/transformer/transformers/ampruntimejs.go +++ b/transformer/transformers/ampruntimejs.go @@ -17,11 +17,14 @@ func AMPRuntimeJS(e *Context) error { if n.Type == html.ElementNode && n.DataAtom == atom.Script { src, ok := htmlnode.FindAttribute(n, "", "src") if ok && strings.HasPrefix(src.Val, amphtml.AMPCacheRootURL) { - u, err := url.Parse(src.Val) - if err != nil { + u, uerr := url.Parse(src.Val) + if uerr != nil { + continue + } + query, queryerr := url.ParseQuery(u.RawQuery) + if queryerr != nil { continue } - query, _ := url.ParseQuery(u.RawQuery) path := u.Path if strings.HasSuffix(path, ".js") { query.Set("f", "sxg") From 2eefaba13c48ce5fbe6a4920b412ea3c8f1c702d Mon Sep 17 00:00:00 2001 From: Derek Tse Date: Sat, 15 Aug 2020 00:57:26 -0700 Subject: [PATCH 26/26] test fix --- transformer/transformers/ampruntimejs_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/transformer/transformers/ampruntimejs_test.go b/transformer/transformers/ampruntimejs_test.go index afffc2d1e..88bee8bec 100644 --- a/transformer/transformers/ampruntimejs_test.go +++ b/transformer/transformers/ampruntimejs_test.go @@ -45,7 +45,7 @@ func TestAmpRuntimeJS(t *testing.T) { { Desc: "additional params exist", Input: ``, - Expected: ``, + Expected: ``, }, { Desc: "existing f param",