From a1aeba22bbded7251fa041ef1d158baff64cfac8 Mon Sep 17 00:00:00 2001 From: Kailash Nadh Date: Sat, 24 Oct 2020 14:03:37 +0530 Subject: [PATCH] Fix invalid link click registrations The link_clicks.link_id table was NULLable incorrectly. Links that do not exist should not register a tracking entry. Fix the query and also update the schema + migration (breaking table change). --- cmd/public.go | 9 +++++---- internal/migrations/v0.8.0.go | 5 +++++ queries.sql | 18 +++++++++--------- schema.sql | 4 ++-- 4 files changed, 21 insertions(+), 15 deletions(-) diff --git a/cmd/public.go b/cmd/public.go index 482dfbfd..1d488c0a 100644 --- a/cmd/public.go +++ b/cmd/public.go @@ -300,13 +300,14 @@ func handleLinkRedirect(c echo.Context) error { var url string if err := app.queries.RegisterLinkClick.Get(&url, linkUUID, campUUID, subUUID); err != nil { - if err != sql.ErrNoRows { - app.log.Printf("error fetching redirect link: %s", err) + if pqErr, ok := err.(*pq.Error); ok && pqErr.Column == "link_id" { + return c.Render(http.StatusNotFound, tplMessage, + makeMsgTpl("Invalid link", "", "The requested link is invalid.")) } + app.log.Printf("error fetching redirect link: %s", err) return c.Render(http.StatusInternalServerError, tplMessage, - makeMsgTpl("Error opening link", "", - "There was an error opening the link. Please try later.")) + makeMsgTpl("Error opening link", "", "There was an error opening the link. Please try later.")) } return c.Redirect(http.StatusTemporaryRedirect, url) diff --git a/internal/migrations/v0.8.0.go b/internal/migrations/v0.8.0.go index 5e7537be..dac2114e 100644 --- a/internal/migrations/v0.8.0.go +++ b/internal/migrations/v0.8.0.go @@ -13,6 +13,11 @@ func V0_8_0(db *sqlx.DB, fs stuffbin.FileSystem, ko *koanf.Koanf) error { ON CONFLICT DO NOTHING; INSERT INTO settings (key, value) VALUES ('messengers', '[]') ON CONFLICT DO NOTHING; + + -- Link clicks shouldn't exist if there's no corresponding link. + -- links_clicks.link_id should have been NOT NULL originally. + DELETE FROM link_clicks WHERE link_id is NULL; + ALTER TABLE link_clicks ALTER COLUMN link_id SET NOT NULL; `) return err } diff --git a/queries.sql b/queries.sql index 8c156f2c..5c8859ae 100644 --- a/queries.sql +++ b/queries.sql @@ -671,16 +671,16 @@ DELETE FROM media WHERE id=$1 RETURNING filename; INSERT INTO links (uuid, url) VALUES($1, $2) ON CONFLICT (url) DO UPDATE SET url=EXCLUDED.url RETURNING uuid; -- name: register-link-click -WITH link AS ( - SELECT url, links.id AS link_id, campaigns.id as campaign_id, subscribers.id AS subscriber_id FROM links - LEFT JOIN campaigns ON (campaigns.uuid = $2) - LEFT JOIN subscribers ON (CASE WHEN $3::TEXT != '' THEN subscribers.uuid = $3::UUID ELSE FALSE END) - WHERE links.uuid = $1 +WITH link AS( + SELECT id, url FROM links WHERE uuid = $1 ) -INSERT INTO link_clicks (campaign_id, subscriber_id, link_id) - VALUES((SELECT campaign_id FROM link), (SELECT subscriber_id FROM link), (SELECT link_id FROM link)) - RETURNING (SELECT url FROM link); - +INSERT INTO link_clicks (campaign_id, subscriber_id, link_id) VALUES( + (SELECT id FROM campaigns WHERE uuid = $2), + (SELECT id FROM subscribers WHERE + (CASE WHEN $3::TEXT != '' THEN subscribers.uuid = $3::UUID ELSE FALSE END) + ), + (SELECT id FROM link) +) RETURNING (SELECT url FROM link); -- name: get-dashboard-charts WITH clicks AS ( diff --git a/schema.sql b/schema.sql index e63924f5..b5ae726e 100644 --- a/schema.sql +++ b/schema.sql @@ -145,8 +145,8 @@ CREATE TABLE links ( DROP TABLE IF EXISTS link_clicks CASCADE; CREATE TABLE link_clicks ( - campaign_id INTEGER REFERENCES campaigns(id) ON DELETE CASCADE ON UPDATE CASCADE, - link_id INTEGER REFERENCES links(id) ON DELETE CASCADE ON UPDATE CASCADE, + campaign_id INTEGER NULL REFERENCES campaigns(id) ON DELETE CASCADE ON UPDATE CASCADE, + link_id INTEGER NOT NULL REFERENCES links(id) ON DELETE CASCADE ON UPDATE CASCADE, -- Subscribers may be deleted, but the link counts should remain. subscriber_id INTEGER NULL REFERENCES subscribers(id) ON DELETE SET NULL ON UPDATE CASCADE,