From 0cb3a45263dc2faa23efc147d1c66b4274a5a8a5 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Fri, 19 Jan 2024 16:23:38 +0100 Subject: [PATCH] cleanup(webconnectivitytle): avoid code duplication (#1456) Use a single algorithm for determining whether there's HTTP diff. We can do this by migrating the HTTP diff flags to use `optional.Value` and then we can always use code written for the "ext" sub-engine to do that. Part of https://github.com/ooni/probe/issues/2640 --- .../webconnectivitylte/analysisclassic.go | 63 ++++++++----------- .../webconnectivitylte/analysisext.go | 2 +- .../webconnectivitylte/analysishttpdiff.go | 41 ++++++++++-- .../experiment/webconnectivitylte/testkeys.go | 16 ++--- 4 files changed, 71 insertions(+), 51 deletions(-) diff --git a/internal/experiment/webconnectivitylte/analysisclassic.go b/internal/experiment/webconnectivitylte/analysisclassic.go index 7347e5f14c..4f96e4be21 100644 --- a/internal/experiment/webconnectivitylte/analysisclassic.go +++ b/internal/experiment/webconnectivitylte/analysisclassic.go @@ -83,28 +83,12 @@ func analysisClassicDNSConsistency(woa *minipipeline.WebAnalysis) optional.Value } func (tk *TestKeys) setHTTPDiffValues(woa *minipipeline.WebAnalysis) { - // TODO(bassosimone): this code should use [newAnalysisHTTPDiffStatus]. - const bodyProportionFactor = 0.7 - if !woa.HTTPFinalResponseDiffBodyProportionFactor.IsNone() { - tk.BodyProportion = woa.HTTPFinalResponseDiffBodyProportionFactor.Unwrap() - value := tk.BodyProportion > bodyProportionFactor - tk.BodyLengthMatch = &value - } - - if !woa.HTTPFinalResponseDiffUncommonHeadersIntersection.IsNone() { - value := len(woa.HTTPFinalResponseDiffUncommonHeadersIntersection.Unwrap()) > 0 - tk.HeadersMatch = &value - } - - if !woa.HTTPFinalResponseDiffStatusCodeMatch.IsNone() { - value := woa.HTTPFinalResponseDiffStatusCodeMatch.Unwrap() - tk.StatusCodeMatch = &value - } - - if !woa.HTTPFinalResponseDiffTitleDifferentLongWords.IsNone() { - value := len(woa.HTTPFinalResponseDiffTitleDifferentLongWords.Unwrap()) <= 0 - tk.TitleMatch = &value - } + hds := newAnalysisHTTPDiffStatus(woa) + tk.BodyProportion = hds.BodyProportion.UnwrapOr(0) + tk.BodyLengthMatch = hds.BodyLengthMatch + tk.HeadersMatch = hds.HeadersMatch + tk.StatusCodeMatch = hds.StatusCodeMatch + tk.TitleMatch = hds.TitleMatch } type analysisClassicTestKeysProxy interface { @@ -128,20 +112,27 @@ var _ analysisClassicTestKeysProxy = &TestKeys{} // httpDiff implements analysisClassicTestKeysProxy. func (tk *TestKeys) httpDiff() bool { - // TODO(bassosimone): this code should use [newAnalysisHTTPDiffStatus]. - if tk.StatusCodeMatch != nil && *tk.StatusCodeMatch { - if tk.BodyLengthMatch != nil && *tk.BodyLengthMatch { - return false - } - if tk.HeadersMatch != nil && *tk.HeadersMatch { - return false - } - if tk.TitleMatch != nil && *tk.TitleMatch { - return false - } - // fallthrough - } - return true + return analysisHTTPDiffAlgorithm(tk) +} + +// bodyLengthMatch implements analysisHTTPDiffValuesProvider. +func (tk *TestKeys) bodyLengthMatch() optional.Value[bool] { + return tk.BodyLengthMatch +} + +// headersMatch implements analysisHTTPDiffValuesProvider. +func (tk *TestKeys) headersMatch() optional.Value[bool] { + return tk.HeadersMatch +} + +// statusCodeMatch implements analysisHTTPDiffValuesProvider. +func (tk *TestKeys) statusCodeMatch() optional.Value[bool] { + return tk.StatusCodeMatch +} + +// titleMatch implements analysisHTTPDiffValuesProvider. +func (tk *TestKeys) titleMatch() optional.Value[bool] { + return tk.TitleMatch } // setBlockingFalse implements analysisClassicTestKeysProxy. diff --git a/internal/experiment/webconnectivitylte/analysisext.go b/internal/experiment/webconnectivitylte/analysisext.go index f25cee14b4..972d75af45 100644 --- a/internal/experiment/webconnectivitylte/analysisext.go +++ b/internal/experiment/webconnectivitylte/analysisext.go @@ -142,7 +142,7 @@ func analysisExtHTTPFinalResponse(tk *TestKeys, analysis *minipipeline.WebAnalys if success := analysis.HTTPFinalResponseSuccessTCPWithControl; !success.IsNone() { txID := success.Unwrap() hds := newAnalysisHTTPDiffStatus(analysis) - if hds.httpDiff() { + if analysisHTTPDiffAlgorithm(hds) { tk.BlockingFlags |= AnalysisBlockingFlagHTTPDiff fmt.Fprintf(info, "- the final response (transaction: %d) differs from the control response\n", txID) return diff --git a/internal/experiment/webconnectivitylte/analysishttpdiff.go b/internal/experiment/webconnectivitylte/analysishttpdiff.go index 8bc2b522f1..d0c89fba32 100644 --- a/internal/experiment/webconnectivitylte/analysishttpdiff.go +++ b/internal/experiment/webconnectivitylte/analysishttpdiff.go @@ -51,16 +51,45 @@ func newAnalysisHTTPDiffStatus(analysis *minipipeline.WebAnalysis) *analysisHTTP return hds } -// httpDiff computes whether there is HTTP diff. -func (hds *analysisHTTPDiffStatus) httpDiff() bool { - if !hds.StatusCodeMatch.IsNone() && hds.StatusCodeMatch.Unwrap() { - if !hds.BodyLengthMatch.IsNone() && hds.BodyLengthMatch.Unwrap() { +type analysisHTTPDiffValuesProvider interface { + bodyLengthMatch() optional.Value[bool] + headersMatch() optional.Value[bool] + statusCodeMatch() optional.Value[bool] + titleMatch() optional.Value[bool] +} + +var _ analysisHTTPDiffValuesProvider = &analysisHTTPDiffStatus{} + +// bodyLengthMatch implements analysisHTTPDiffValuesProvider. +func (hds *analysisHTTPDiffStatus) bodyLengthMatch() optional.Value[bool] { + return hds.BodyLengthMatch +} + +// headersMatch implements analysisHTTPDiffValuesProvider. +func (hds *analysisHTTPDiffStatus) headersMatch() optional.Value[bool] { + return hds.HeadersMatch +} + +// statusCodeMatch implements analysisHTTPDiffValuesProvider. +func (hds *analysisHTTPDiffStatus) statusCodeMatch() optional.Value[bool] { + return hds.StatusCodeMatch +} + +// titleMatch implements analysisHTTPDiffValuesProvider. +func (hds *analysisHTTPDiffStatus) titleMatch() optional.Value[bool] { + return hds.TitleMatch +} + +// analysisHTTPDiffAlgorithm returns whether there's an HTTP diff +func analysisHTTPDiffAlgorithm(p analysisHTTPDiffValuesProvider) bool { + if !p.statusCodeMatch().IsNone() && p.statusCodeMatch().Unwrap() { + if !p.bodyLengthMatch().IsNone() && p.bodyLengthMatch().Unwrap() { return false } - if !hds.HeadersMatch.IsNone() && hds.HeadersMatch.Unwrap() { + if !p.headersMatch().IsNone() && p.headersMatch().Unwrap() { return false } - if !hds.TitleMatch.IsNone() && hds.TitleMatch.Unwrap() { + if !p.titleMatch().IsNone() && p.titleMatch().Unwrap() { return false } // fallthrough diff --git a/internal/experiment/webconnectivitylte/testkeys.go b/internal/experiment/webconnectivitylte/testkeys.go index bf854714c3..2674836bb1 100644 --- a/internal/experiment/webconnectivitylte/testkeys.go +++ b/internal/experiment/webconnectivitylte/testkeys.go @@ -99,16 +99,16 @@ type TestKeys struct { BodyProportion float64 `json:"body_proportion"` // BodyLength match tells us whether the body length matches. - BodyLengthMatch *bool `json:"body_length_match"` + BodyLengthMatch optional.Value[bool] `json:"body_length_match"` // HeadersMatch tells us whether the headers match. - HeadersMatch *bool `json:"headers_match"` + HeadersMatch optional.Value[bool] `json:"headers_match"` // StatusCodeMatch tells us whether the status code matches. - StatusCodeMatch *bool `json:"status_code_match"` + StatusCodeMatch optional.Value[bool] `json:"status_code_match"` // TitleMatch tells us whether the title matches. - TitleMatch *bool `json:"title_match"` + TitleMatch optional.Value[bool] `json:"title_match"` // Blocking indicates the reason for blocking. This is notoriously a bad // type because it can be one of the following values: @@ -363,10 +363,10 @@ func NewTestKeys() *TestKeys { BlockingFlags: 0, NullNullFlags: 0, BodyProportion: 0, - BodyLengthMatch: nil, - HeadersMatch: nil, - StatusCodeMatch: nil, - TitleMatch: nil, + BodyLengthMatch: optional.None[bool](), + HeadersMatch: optional.None[bool](), + StatusCodeMatch: optional.None[bool](), + TitleMatch: optional.None[bool](), Blocking: nil, Accessible: nil, ControlRequest: nil,