feat: override socket.url with patched.url for all imports #25

omegabytes · 2022-08-19T17:45:48Z

Updates socket.lua to return patched/url.lua so that imports of socket.url are overridden to implement patched.url instead.
Prior to this change, plugins using socket.url would need to be manually patched in order to function. By overriding the import, we ensure that custom plugins using socket.url work out-of-the-box.

Adds a test specifically for the socket.url <-> patched.url swap, and updates the diff in lua-tree.patch to reflect accurate diff.

Hold until after #27

CLAassistant · 2022-08-19T17:45:52Z

All committers have signed the CLA.

omegabytes · 2022-08-22T17:56:56Z

related to #26

mikefero

Starting to come to shape; testing and regression issues currently.

mikefero · 2022-08-23T11:30:18Z

plugin/testdata/patched_url_test.lua

+            },
+        },
+        },
+    },}


This schema needs more for the test; url is never used so there is no validation happening here. The current field uuid has no value for url. Create a custom_validator function to verify that url.parse(<a-url-field>) is working as expected:

<url> ::= <scheme>://<authority>/<path>;<params>?<query>#<fragment> <authority> ::= <userinfo>@<host>:<port> <userinfo> ::= <user>[:<password>] <path> :: = {<segment>/}<segment>

You will want to check:

scheme

authority

userinfo

user

password

host

port

path

params

query

fragment

This test could also be extended to validated patched.url as well in the validator that way we are ensuring backwards compatibility through a regression.

mikefero · 2022-08-23T11:30:50Z

plugin/validator_test.go

+		pluginName, err := v.LoadSchema(string(schema))
+		assert.Equal(t, "patched-url-test", pluginName)
+		assert.Nil(t, err)
+	})


See comment in test schema; we need to increase the surface area of this test.

mikefero · 2022-08-23T11:32:32Z

patches/lua-tree.patch

+-
+-return _M
+return require("patched.url")
+


I haven't done so yet, but did you ensure the lua-tree.patch is still working correctly?

patches/lua-tree.patch

lua-tree/share/lua/5.1/socket.lua

omegabytes · 2022-09-07T21:31:57Z

@mikefero patch file is correct now, tests passing.

mikefero · 2022-09-08T12:56:12Z

patches/lua-tree.patch

+lua-tree/share/lua/5.1/socket/url.lua
+- override all imports of socket.url by returning patched.url
+
+


Duplicated at the bottom; we can remove this.

Suggested change

lua-tree/share/lua/5.1/socket/url.lua

- override all imports of socket.url by returning patched.url

mikefero · 2022-09-08T12:57:11Z

patches/lua-tree.patch

@@ -72,6 +69,9 @@ tools/sandbox.lua
 - set configuration.enabled to true
 - set configuration.sandbox_enabled to true

+lua-tree/share/lua/5.1/socket/url.lua


super duper nit: between each file we are using two newlines

Suggested change

lua-tree/share/lua/5.1/socket/url.lua

lua-tree/share/lua/5.1/socket/url.lua

mikefero · 2022-09-12T17:36:25Z

@omegabytes Let's get #26 in first and then we can rebase this and fix it up; WDYT?

mikefero · 2022-09-14T14:00:49Z

Found one more usage of patched.url in the patch file that needs to be removed:

diff --git a/patches/lua-tree.patch b/patches/lua-tree.patch
index 5953b79..eb93b27 100644
--- a/patches/lua-tree.patch
+++ b/patches/lua-tree.patch
@@ -683,15 +683,6 @@ index 08405b8..2b1157d 100644
  end

  -- function below is more acurate, but invalidates previously accepted uuids and hence causes
-@@ -273,7 +133,7 @@ end
- --end
-
- do
--  local url = require "socket.url"
-+  local url = require "patched.url"
-
-   --- URL escape and format key and value
-   -- values should be already decoded or the `raw` option should be passed to prevent double-encoding
 @@ -508,7 +368,18 @@ do
    local floor = math.floor
    local max = math.max

Updates socket.lua to return patched/url.lua so that imports of socket.url are overridden to implement patched.url instead. Prior to this change, plugins using socket.url would need to be manually patched in order to function. By overriding the import, we ensure that custom plugins using socket.url work out-of-the-box.

mikefero

Nice work!!!

omegabytes requested a review from a team as a code owner August 19, 2022 17:45

omegabytes requested a review from mikefero August 19, 2022 17:45

omegabytes force-pushed the feat/use-patched-url-by-default branch from 76fd29a to 8c18263 Compare August 19, 2022 18:21

hbagdi requested a review from javierguerragiraldez August 19, 2022 21:39

omegabytes mentioned this pull request Aug 22, 2022

feat: alias glaure.match as ngx.re.match #26

Merged

mikefero reviewed Aug 23, 2022

View reviewed changes

omegabytes commented Sep 6, 2022

View reviewed changes

patches/lua-tree.patch Show resolved Hide resolved

omegabytes force-pushed the feat/use-patched-url-by-default branch from d8b0c74 to ad9d9de Compare September 6, 2022 18:51

omegabytes commented Sep 6, 2022

View reviewed changes

lua-tree/share/lua/5.1/socket.lua Outdated Show resolved Hide resolved

omegabytes force-pushed the feat/use-patched-url-by-default branch 3 times, most recently from 4c3740f to 6104892 Compare September 7, 2022 21:29

omegabytes requested a review from mikefero September 7, 2022 21:30

omegabytes force-pushed the feat/use-patched-url-by-default branch from 6104892 to 4edd380 Compare September 7, 2022 21:45

mikefero reviewed Sep 8, 2022

View reviewed changes

omegabytes added the hold/do-not-merge label Sep 8, 2022

omegabytes force-pushed the feat/use-patched-url-by-default branch from ed9c6b7 to f15d88e Compare September 13, 2022 23:00

omegabytes requested a review from mikefero September 13, 2022 23:02

mikefero removed the hold/do-not-merge label Sep 14, 2022

omegabytes force-pushed the feat/use-patched-url-by-default branch from f15d88e to d5dcad5 Compare September 14, 2022 16:30

mikefero approved these changes Sep 14, 2022

View reviewed changes

mikefero merged commit a40c4d2 into main Sep 14, 2022

mikefero deleted the feat/use-patched-url-by-default branch September 14, 2022 18:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: override socket.url with patched.url for all imports #25

feat: override socket.url with patched.url for all imports #25

omegabytes commented Aug 19, 2022 •

edited

Loading

CLAassistant commented Aug 19, 2022 •

edited

Loading

omegabytes commented Aug 22, 2022

mikefero left a comment

mikefero Aug 23, 2022

mikefero Aug 23, 2022

mikefero Aug 23, 2022

omegabytes commented Sep 7, 2022

mikefero Sep 8, 2022

mikefero Sep 8, 2022

mikefero commented Sep 12, 2022

mikefero commented Sep 14, 2022

mikefero left a comment

		lua-tree/share/lua/5.1/socket/url.lua
		- override all imports of socket.url by returning patched.url

feat: override socket.url with patched.url for all imports #25

feat: override socket.url with patched.url for all imports #25

Conversation

omegabytes commented Aug 19, 2022 • edited Loading

CLAassistant commented Aug 19, 2022 • edited Loading

omegabytes commented Aug 22, 2022

mikefero left a comment

Choose a reason for hiding this comment

mikefero Aug 23, 2022

Choose a reason for hiding this comment

mikefero Aug 23, 2022

Choose a reason for hiding this comment

mikefero Aug 23, 2022

Choose a reason for hiding this comment

omegabytes commented Sep 7, 2022

mikefero Sep 8, 2022

Choose a reason for hiding this comment

mikefero Sep 8, 2022

Choose a reason for hiding this comment

mikefero commented Sep 12, 2022

mikefero commented Sep 14, 2022

mikefero left a comment

Choose a reason for hiding this comment

omegabytes commented Aug 19, 2022 •

edited

Loading

CLAassistant commented Aug 19, 2022 •

edited

Loading