From 3dcd2deb86feee6efe64359ce83fc39525f75822 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Sun, 18 Oct 2026 14:15:20 +0200 Subject: [PATCH] Suggest article selectors with OpenAI on request When a page is parsed wrong, an administrator can ask OpenAI for a CSS selector of the article. The model sees an outline of the page body instead of raw HTML, and a suggestion is accepted only after it is applied to the page and matches a plausible amount of text that is neither nearly the whole page nor what is extracted now. The selector is stored as a disabled rule with an insert-only write, so an existing rule of the domain is never replaced and nothing changes for readers until the rule is enabled. OpenAI is never asked for a selector during normal extraction. The request shares the per-request deadline used for summaries. --- CLAUDE.md | 5 +- README.md | 9 +- datastore/rules.go | 40 ++++-- datastore/rules_test.go | 43 +++++++ extractor/improve.go | 206 +++++++++++++++++++++++++++++ extractor/improve_test.go | 263 ++++++++++++++++++++++++++++++++++++++ extractor/mocks/rules.go | 60 ++++++++- extractor/outline.go | 174 +++++++++++++++++++++++++ extractor/outline_test.go | 72 +++++++++++ extractor/readability.go | 47 +++++-- extractor/summary.go | 4 +- rest/server.go | 40 +++++- rest/server_test.go | 137 +++++++++++++++++++- 13 files changed, 1055 insertions(+), 45 deletions(-) create mode 100644 extractor/improve.go create mode 100644 extractor/improve_test.go create mode 100644 extractor/outline.go create mode 100644 extractor/outline_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 02d399fd..688dd1b9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -39,9 +39,10 @@ web/ → Go HTML templates (HTMX v2), static assets **Key interfaces:** - `extractor.Rules` (defined consumer-side in `extractor/readability.go`), implemented by `datastore.RulesDAO`. Mock generated with `//go:generate moq` in extractor package. - `extractor.Retriever` (defined in `extractor/retriever.go`) — abstracts URL content fetching. Two implementations: `HTTPRetriever` (default, standard HTTP GET with Safari user-agent) and `CloudflareRetriever` (Cloudflare Browser Rendering API for JS-rendered pages). When `UReadability.Retriever` is nil, defaults to `HTTPRetriever`. -- `extractor.OpenAIClient` and `extractor.Summaries` (defined in `extractor/summary.go`): the go-openai client and the summary cache (`datastore.SummariesDAO`, TTL index on `expires_at`). `UReadability.OpenAI` is set only when `--openai.api-key` is given; nil means summaries are off. Mocks in `extractor/mocks/`; moq cannot read Go 1.27 export data, so regenerate them with a newer moq or edit by hand. +- `extractor.SuggestRule` (`extractor/improve.go`) backs `POST /api/content-parsed-wrong`: the model suggests a selector from a page outline (`extractor/outline.go`), the selector is checked against the page, and it is stored as a disabled rule when the domain has none. It never runs during normal extraction. +- `extractor.OpenAIClient` and `extractor.Summaries` (defined in `extractor/summary.go`): the go-openai client and the summary cache (`datastore.SummariesDAO`, TTL index on `expires_at`). `UReadability.OpenAI` is set only when `--openai.api-key` is given; nil means OpenAI features (summaries, `SuggestRule`) are off. Mocks in `extractor/mocks/`; moq cannot read Go 1.27 export data, so regenerate them with a newer moq or edit by hand. -Optional OpenAI flags, all in the `--openai.*` group: `api-key` / `OPENAI_API_KEY` (enables summaries), `model` / `OPENAI_MODEL` (default `gpt-6-luna`), `summary-prompt`, `max-content-length` (characters, default 10000). +Optional OpenAI flags, all in the `--openai.*` group: `api-key` / `OPENAI_API_KEY` (enables summaries and selector suggestions), `model` / `OPENAI_MODEL` (default `gpt-6-luna`), `summary-prompt`, `max-content-length` (characters, default 10000). ## Content Extraction Flow diff --git a/README.md b/README.md index e83117f4..a94b11c7 100644 --- a/README.md +++ b/README.md @@ -21,13 +21,13 @@ | cf-route-all | CF_ROUTE_ALL | `false` | route every request through Cloudflare Browser Rendering | | dbg | DEBUG | `false` | debug mode | -#### OpenAI summaries (optional) +#### OpenAI (optional) -Setting `--openai.api-key` enables article summaries; without it no OpenAI calls are made. +Setting `--openai.api-key` enables article summaries and selector suggestions for pages that are parsed wrong; without it no OpenAI calls are made. | Command line | Environment | Default | Description | |---------------------------|---------------------------|--------------|--------------------------------------------------------------------| -| openai.api-key | OPENAI_API_KEY | none | OpenAI API key, enables summaries | +| openai.api-key | OPENAI_API_KEY | none | OpenAI API key, enables summaries and selector suggestions | | openai.model | OPENAI_MODEL | `gpt-6-luna` | OpenAI model | | openai.summary-prompt | OPENAI_SUMMARY_PROMPT | built-in | custom system prompt for summaries | | openai.max-content-length | OPENAI_MAX_CONTENT_LENGTH | `10000` | max characters of article text sent for summarizing (0 for no limit) | @@ -46,9 +46,12 @@ When Cloudflare credentials are not set, the service uses a standard HTTP client GET /api/content/v1/parser?token=secret&url=http://aa.com/blah - extract content (emulate Readability API parse call) GET /api/content/v1/parser?token=secret&url=http://aa.com/blah&summary=true - extract content with AI-generated summary POST /api/extract {url: http://aa.com/blah} - extract content + POST /api/content-parsed-wrong?url=http://aa.com/blah - ask OpenAI for a rule for a page that is parsed wrong (credentials required) Summary generation requires the server token to be set and an OpenAI API key. Summaries are cached in MongoDB for 30 days, keyed by model, prompt and article text; a TTL index removes expired ones. When the key is set, rule previews in the web UI also show a summary. +`POST /api/content-parsed-wrong` uses the same credentials as the rule management routes and calls OpenAI only when it is requested, never during normal extraction. The model gets an outline of the page body and suggests a CSS selector for the article. The selector is applied to the page and accepted only when it matches a plausible amount of text, not nearly the whole page and not what is extracted now; up to three suggestions are tried. The response holds the selector, the page extracted with it and `saved`. When the domain has no rule yet, the selector is stored as a **disabled** rule, so nothing changes for readers until it is reviewed and enabled in the web UI. When the domain already has a rule, enabled or not, that rule is left as it is and only the suggestion is returned. + ## Development ### Running tests diff --git a/datastore/rules.go b/datastore/rules.go index 12a5da3f..7459112a 100644 --- a/datastore/rules.go +++ b/datastore/rules.go @@ -18,6 +18,7 @@ const ( fieldID = "_id" fieldExpiresAt = "expires_at" opSet = "$set" + opSetOnInsert = "$setOnInsert" ) // RulesDAO data-access obj for custom parsing rules, implements Rules @@ -77,17 +78,7 @@ func (r RulesDAO) GetByID(ctx context.Context, id bson.ObjectID) (Rule, bool) { // Save upsert rule func (r RulesDAO) Save(ctx context.Context, rule Rule) (Rule, error) { - // normalise nil slices to empty ones so cleared multi-value fields are stored as BSON arrays - // ([]) rather than null (the driver encodes nil slices as null), keeping the stored type stable. - if rule.MatchURLs == nil { - rule.MatchURLs = []string{} - } - if rule.Excludes == nil { - rule.Excludes = []string{} - } - if rule.TestURLs == nil { - rule.TestURLs = []string{} - } + rule = withEmptySlices(rule) ch, err := r.UpdateOne(ctx, bson.M{fieldDomain: rule.Domain}, bson.M{opSet: rule}, options.UpdateOne().SetUpsert(true)) if err != nil { log.Printf("[WARN] failed to save, error=%v, article=%v", err, rule) @@ -109,6 +100,33 @@ func (r RulesDAO) Save(ctx context.Context, rule Rule) (Rule, error) { return rule, err } +// SaveIfAbsent stores the rule only when the domain has no rule yet, enabled or not, and reports +// whether it did. An existing rule is never touched, unlike Save which replaces it. +func (r RulesDAO) SaveIfAbsent(ctx context.Context, rule Rule) (bool, error) { + rule = withEmptySlices(rule) + ch, err := r.UpdateOne(ctx, bson.M{fieldDomain: rule.Domain}, bson.M{opSetOnInsert: rule}, options.UpdateOne().SetUpsert(true)) + if err != nil { + log.Printf("[WARN] failed to save, error=%v, article=%v", err, rule) + return false, err + } + return ch.UpsertedID != nil, nil +} + +// withEmptySlices normalises nil slices to empty ones so cleared multi-value fields are stored as BSON +// arrays ([]) rather than null (the driver encodes nil slices as null), keeping the stored type stable. +func withEmptySlices(rule Rule) Rule { + if rule.MatchURLs == nil { + rule.MatchURLs = []string{} + } + if rule.Excludes == nil { + rule.Excludes = []string{} + } + if rule.TestURLs == nil { + rule.TestURLs = []string{} + } + return rule +} + // Disable marks enabled=false, by id func (r RulesDAO) Disable(ctx context.Context, id bson.ObjectID) error { _, err := r.UpdateOne(ctx, bson.M{fieldID: id}, bson.M{opSet: bson.M{fieldEnabled: false}}) diff --git a/datastore/rules_test.go b/datastore/rules_test.go index 19ca4d86..26aa32f8 100644 --- a/datastore/rules_test.go +++ b/datastore/rules_test.go @@ -2,6 +2,7 @@ package datastore import ( "context" + "fmt" "math/rand/v2" "testing" @@ -92,6 +93,48 @@ func TestRulesSave(t *testing.T) { }) } +func TestRulesSaveIfAbsent(t *testing.T) { + rules := setupRules(t) + ctx := context.Background() + + t.Run("new domain is stored", func(t *testing.T) { + rule := Rule{Domain: randDomain(), Content: "div.copy", User: "openai", TestURLs: []string{"https://example.com/a"}} + saved, err := rules.SaveIfAbsent(ctx, rule) + require.NoError(t, err) + assert.True(t, saved) + + var stored Rule + require.NoError(t, rules.FindOne(ctx, bson.M{fieldDomain: rule.Domain}).Decode(&stored)) + assert.Equal(t, "div.copy", stored.Content) + assert.Equal(t, "openai", stored.User) + assert.False(t, stored.Enabled) + assert.Equal(t, []string{"https://example.com/a"}, stored.TestURLs) + assert.Equal(t, []string{}, stored.MatchURLs, "nil slices are stored as arrays") + }) + + for _, enabled := range []bool{true, false} { + t.Run(fmt.Sprintf("existing rule is kept, enabled=%v", enabled), func(t *testing.T) { + domain := randDomain() + orig, err := rules.Save(ctx, Rule{Domain: domain, Content: "original", Author: "me", Enabled: enabled}) + require.NoError(t, err) + + saved, err := rules.SaveIfAbsent(ctx, Rule{Domain: domain, Content: "suggested", User: "openai"}) + require.NoError(t, err) + assert.False(t, saved) + + var stored []Rule + cursor, err := rules.Find(ctx, bson.M{fieldDomain: domain}) + require.NoError(t, err) + require.NoError(t, cursor.All(ctx, &stored)) + require.Len(t, stored, 1) + assert.Equal(t, orig.ID, stored[0].ID) + assert.Equal(t, "original", stored[0].Content) + assert.Equal(t, "me", stored[0].Author) + assert.Equal(t, enabled, stored[0].Enabled) + }) + } +} + func TestRulesGet(t *testing.T) { rules := setupRules(t) diff --git a/extractor/improve.go b/extractor/improve.go new file mode 100644 index 00000000..43adf64e --- /dev/null +++ b/extractor/improve.go @@ -0,0 +1,206 @@ +package extractor + +import ( + "cmp" + "context" + "encoding/json" + "errors" + "fmt" + "net/url" + "strings" + "time" + + "github.com/PuerkitoBio/goquery" + log "github.com/go-pkgz/lgr" + "github.com/sashabaranov/go-openai" + + "github.com/ukeeper/ukeeper-readability/datastore" +) + +// ErrOpenAIDisabled is returned by SuggestRule when no OpenAI client is configured +var ErrOpenAIDisabled = errors.New("OpenAI is not configured") + +// ErrNoSelector is returned by SuggestRule when no attempt produced a selector that passed the checks +var ErrNoSelector = errors.New("no usable selector was suggested") + +// errInvalidAnswer marks a model reply that is not the requested JSON +var errInvalidAnswer = errors.New("reply is not the requested JSON") + +const ( + suggestAttempts = 3 + suggestCallTimeout = 60 * time.Second // one model call + suggestedRuleUser = "openai" + maxSelectorLen = 200 + minSuggestedLen = 200 // characters of text a selector has to match + maxWholePagePercent = 90 // a selector matching more of the page text than this is the page, not the article + currentTextLen = 500 // characters of the current extraction shown to the model +) + +const suggestPrompt = `You find the main article on a web page. You get an outline of the page body: one line per element ` + + `with its tag, id, classes, the number of characters of visible text and the number of paragraphs. ` + + `Reply with JSON only: {"selector": ""}. ` + + `The selector must match the element that holds the whole article text and nothing else: ` + + `no navigation, comments, related links, share buttons or footer. ` + + `Prefer an id or a stable class over positional selectors.` + +// Suggestion is what SuggestRule found for a URL +type Suggestion struct { + Selector string `json:"selector"` + Saved bool `json:"saved"` // true when the selector was stored as a disabled rule for review + Response *Response `json:"response"` // the page extracted with the selector +} + +// attempt is a rejected selector together with the reason, passed back to the model +type attempt struct { + selector string + reason string +} + +// SuggestRule asks the model for a CSS selector of the article on the page, for a page the general +// parser got wrong. The selector is applied to the page and has to match a plausible amount of text +// before it is accepted. When the domain has no rule yet, it is stored as a disabled rule, so +// nothing changes for readers until someone reviews and enables it. When the domain already has a +// rule, the existing one is left alone and the suggestion is only returned. +func (f *UReadability) SuggestRule(ctx context.Context, reqURL string) (*Suggestion, error) { + if f.OpenAI == nil { + return nil, ErrOpenAIDisabled + } + u, err := url.Parse(reqURL) + if err != nil { + return nil, fmt.Errorf("parse url %q: %w", reqURL, err) + } + + var rule *datastore.Rule // the enabled rule, as extraction would use it + if f.Rules != nil { + if r, found := f.Rules.Get(ctx, reqURL); found { + rule = &r + } + } + + rb, body, err := f.fetch(ctx, reqURL, rule) + if err != nil { + return nil, err + } + doc, err := goquery.NewDocumentFromReader(strings.NewReader(body)) + if err != nil { + return nil, err + } + + current, _, _ := f.getContent(ctx, body, reqURL, rule) + current = f.getText(current, "") + outline := pageOutline(doc.Nodes[0]) + pageLen := visibleTextLen(findBody(doc.Nodes[0])) + + var rejected []attempt + for range suggestAttempts { + selector, err := f.askSelector(ctx, reqURL, current, outline, rejected) + if errors.Is(err, errInvalidAnswer) { + rejected = append(rejected, attempt{reason: "the reply was not the requested JSON"}) + continue + } + if err != nil { + return nil, err + } + if reason := f.checkSelector(body, selector, current, pageLen); reason != "" { + log.Printf("[INFO] selector %q for %s rejected: %s", selector, reqURL, reason) + rejected = append(rejected, attempt{selector: selector, reason: reason}) + continue + } + + resp, err := f.parse(ctx, reqURL, body, rb, &datastore.Rule{Content: selector, Enabled: true}) + if err != nil { + return nil, err + } + return &Suggestion{Selector: selector, Saved: f.saveSuggested(ctx, u.Host, reqURL, selector), Response: resp}, nil + } + return nil, ErrNoSelector +} + +// askSelector makes one model call and returns the selector from the reply +func (f *UReadability) askSelector(ctx context.Context, reqURL, current, outline string, rejected []attempt) (string, error) { + var sb strings.Builder + fmt.Fprintf(&sb, "URL: %s\n\n", reqURL) + if current != "" { + if r := []rune(current); len(r) > currentTextLen { + current = string(r[:currentTextLen]) + } + fmt.Fprintf(&sb, "The text extracted now is reported as wrong. It starts with:\n---\n%s\n---\n\n", current) + } + fmt.Fprintf(&sb, "Page body outline:\n---\n%s\n---\n", outline) + for _, a := range rejected { + if a.selector == "" { + fmt.Fprintf(&sb, "\nAn earlier attempt failed: %s. Try again.", a.reason) + continue + } + fmt.Fprintf(&sb, "\nThe selector %q was rejected: %s. Suggest a different one.", a.selector, a.reason) + } + + callCtx, cancel := context.WithTimeout(ctx, suggestCallTimeout) + defer cancel() + resp, err := f.OpenAI.CreateChatCompletion(callCtx, openai.ChatCompletionRequest{ + Model: cmp.Or(f.OpenAIModel, DefaultOpenAIModel), + Messages: []openai.ChatCompletionMessage{ + {Role: openai.ChatMessageRoleSystem, Content: suggestPrompt}, + {Role: openai.ChatMessageRoleUser, Content: sb.String()}, + }, + ResponseFormat: &openai.ChatCompletionResponseFormat{Type: openai.ChatCompletionResponseFormatTypeJSONObject}, + }) + if err != nil { + return "", fmt.Errorf("openai selector request: %w", err) + } + if len(resp.Choices) == 0 { + return "", fmt.Errorf("openai returned no choices: %w", errInvalidAnswer) + } + + var answer struct { + Selector string `json:"selector"` + } + if err := json.Unmarshal([]byte(resp.Choices[0].Message.Content), &answer); err != nil || strings.TrimSpace(answer.Selector) == "" { + return "", errInvalidAnswer + } + return strings.TrimSpace(answer.Selector), nil +} + +// checkSelector applies the selector to the page and returns why it can't be the article, or "" when it can +func (f *UReadability) checkSelector(body, selector, current string, pageLen int) string { + if len(selector) > maxSelectorLen { + return "it is too long" + } + res, err := extractWithSelector(body, selector) + if err != nil || res == "" { + return "it matches nothing on the page" + } + doc, err := goquery.NewDocumentFromReader(strings.NewReader(res)) + if err != nil { + return "what it matches can't be parsed" + } + n := visibleTextLen(findBody(doc.Nodes[0])) + switch { + case n < minSuggestedLen: + return fmt.Sprintf("it matches only %d characters of text", n) + case n*100 > pageLen*maxWholePagePercent: + return "it matches almost all the text of the page" + case f.getText(res, "") == current: + return "it extracts the same text as the current result" + } + return "" +} + +// saveSuggested stores the selector as a disabled rule when the domain has none. Both enabled and +// disabled rules count, and an existing rule is never replaced. +func (f *UReadability) saveSuggested(ctx context.Context, domain, reqURL, selector string) bool { + if f.Rules == nil { + return false + } + rule := datastore.Rule{Domain: domain, Content: selector, TestURLs: []string{reqURL}, User: suggestedRuleUser} + saved, err := f.Rules.SaveIfAbsent(ctx, rule) + switch { + case err != nil: + log.Printf("[WARN] failed to save suggested rule for %s: %v", domain, err) + case !saved: + log.Printf("[INFO] %s already has a rule, suggested selector %q is not saved", domain, selector) + default: + log.Printf("[INFO] saved disabled rule for %s with selector %q", domain, selector) + } + return err == nil && saved +} diff --git a/extractor/improve_test.go b/extractor/improve_test.go new file mode 100644 index 00000000..48fd70fc --- /dev/null +++ b/extractor/improve_test.go @@ -0,0 +1,263 @@ +package extractor + +import ( + "context" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + "github.com/sashabaranov/go-openai" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ukeeper/ukeeper-readability/datastore" + "github.com/ukeeper/ukeeper-readability/extractor/mocks" +) + +// articleText is long enough to pass the minimum length check +var articleText = strings.Repeat("The quick brown fox jumps over the lazy dog. ", 12) + +// commentsText is the kind of text the general parser prefers over the article: long, comma-rich paragraphs +var commentsText = strings.Repeat("Well, I think so, but then again, who knows, maybe not, and yet here we are, arguing about it, again. ", 6) + +// storyPage keeps the article in div.copy, followed by a bigger block of comments that the general +// parser picks instead. div#page and body hold all of it, so both are too wide to be the article. +func storyPage() string { + comments := "" + for range 8 { + comments += "

" + commentsText + "

" + } + return `Story title
+
  • Home
  • News
  • Contact
+

` + articleText + `

` + articleText + `

` + articleText + `

+
` + comments + `
+
` +} + +func jsonSelector(selector string) string { + return fmt.Sprintf(`{"selector": %q}`, selector) +} + +// sequence answers each call with the next reply +func sequence(replies ...string) func(context.Context, openai.ChatCompletionRequest) (openai.ChatCompletionResponse, error) { + var i int + return func(ctx context.Context, req openai.ChatCompletionRequest) (openai.ChatCompletionResponse, error) { + reply := replies[min(i, len(replies)-1)] + i++ + return answer(reply)(ctx, req) + } +} + +func suggestServer(t *testing.T, page string) *httptest.Server { + t.Helper() + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(page)) + })) + t.Cleanup(ts.Close) + return ts +} + +// rulesStore is a Rules mock that records SaveIfAbsent calls and refuses to save over a rule of the +// same domain, as the datastore does; existing are the rules the domain already has. Save is not set, +// so a call to it panics: replacing a rule is never allowed here. +func rulesStore(existing ...datastore.Rule) *mocks.RulesMock { + return &mocks.RulesMock{ + GetFunc: func(_ context.Context, rURL string) (datastore.Rule, bool) { + u, _ := url.Parse(rURL) + for _, r := range existing { + if r.Domain == u.Host && r.Enabled { + return r, true + } + } + return datastore.Rule{}, false + }, + SaveIfAbsentFunc: func(_ context.Context, rule datastore.Rule) (bool, error) { + for _, r := range existing { + if r.Domain == rule.Domain { + return false, nil + } + } + return true, nil + }, + } +} + +func newSuggester(client OpenAIClient, rules Rules) *UReadability { + return &UReadability{TimeOut: 5 * time.Second, SnippetSize: 100, OpenAI: client, Rules: rules} +} + +func TestSuggestRule(t *testing.T) { + t.Run("disabled without client", func(t *testing.T) { + r := UReadability{} + _, err := r.SuggestRule(context.Background(), "http://example.com/a") + assert.ErrorIs(t, err, ErrOpenAIDisabled) + }) + + t.Run("good selector is stored disabled and used for the response", func(t *testing.T) { + ts := suggestServer(t, storyPage()) + client := &mocks.OpenAIClientMock{CreateChatCompletionFunc: answer(jsonSelector("div.copy"))} + rules := rulesStore() + + res, err := newSuggester(client, rules).SuggestRule(context.Background(), ts.URL+"/article") + require.NoError(t, err) + assert.Equal(t, "div.copy", res.Selector) + assert.True(t, res.Saved) + assert.Contains(t, res.Response.Content, "The quick brown fox") + assert.NotContains(t, res.Response.Content, "Well, I think so") + assert.NotContains(t, res.Response.Content, "Contact") + + require.Len(t, rules.SaveIfAbsentCalls(), 1) + saved := rules.SaveIfAbsentCalls()[0].Rule + host, _ := url.Parse(ts.URL) + assert.Equal(t, datastore.Rule{Domain: host.Host, Content: "div.copy", TestURLs: []string{ts.URL + "/article"}, User: "openai"}, saved) + assert.False(t, saved.Enabled, "an unreviewed rule must not affect readers") + + require.Len(t, client.CreateChatCompletionCalls(), 1) + req := client.CreateChatCompletionCalls()[0].Request + assert.Equal(t, openai.GPT6Luna, req.Model) + assert.Equal(t, openai.ChatCompletionResponseFormatTypeJSONObject, req.ResponseFormat.Type) + assert.Zero(t, req.MaxCompletionTokens, "reasoning models reject token limits") + assert.Zero(t, req.MaxTokens, "reasoning models reject max_tokens") //nolint:staticcheck // the validator rejects the deprecated field + assert.Zero(t, req.Temperature, "reasoning models reject temperature") + require.Len(t, req.Messages, 2) + prompt := req.Messages[1].Content + assert.Contains(t, prompt, ts.URL+"/article") + assert.Contains(t, prompt, "div.copy text=") + assert.NotContains(t, prompt, "tracking", "script text is not part of the outline") + }) + + t.Run("custom model", func(t *testing.T) { + ts := suggestServer(t, storyPage()) + client := &mocks.OpenAIClientMock{CreateChatCompletionFunc: answer(jsonSelector("div.copy"))} + r := newSuggester(client, rulesStore()) + r.OpenAIModel = "other-model" + _, err := r.SuggestRule(context.Background(), ts.URL+"/article") + require.NoError(t, err) + assert.Equal(t, "other-model", client.CreateChatCompletionCalls()[0].Request.Model) + }) + + rejected := []struct { + name string + reply string + reason string + }{ + {"matches nothing", jsonSelector("div.nothing"), `"div.nothing" was rejected: it matches nothing on the page`}, + {"too little text", jsonSelector("ul.menu"), `"ul.menu" was rejected: it matches only`}, + {"almost the whole page", jsonSelector("div#page"), `"div#page" was rejected: it matches almost all the text of the page`}, + {"too long", jsonSelector("div" + strings.Repeat(".copy", 50)), "was rejected: it is too long"}, + {"not json", "div.copy", "an earlier attempt failed: the reply was not the requested JSON"}, + {"empty selector", jsonSelector(" "), "an earlier attempt failed: the reply was not the requested JSON"}, + } + for _, tt := range rejected { + t.Run("rejected then retried: "+tt.name, func(t *testing.T) { + ts := suggestServer(t, storyPage()) + client := &mocks.OpenAIClientMock{CreateChatCompletionFunc: sequence(tt.reply, jsonSelector("div.copy"))} + rules := rulesStore() + + res, err := newSuggester(client, rules).SuggestRule(context.Background(), ts.URL+"/article") + require.NoError(t, err) + assert.Equal(t, "div.copy", res.Selector) + + require.Len(t, client.CreateChatCompletionCalls(), 2) + assert.Contains(t, strings.ToLower(client.CreateChatCompletionCalls()[1].Request.Messages[1].Content), strings.ToLower(tt.reason)) + require.Len(t, rules.SaveIfAbsentCalls(), 1) + assert.Equal(t, "div.copy", rules.SaveIfAbsentCalls()[0].Rule.Content, "a rejected selector must never be stored") + }) + } + + t.Run("same text as the current result is rejected", func(t *testing.T) { + ts := suggestServer(t, storyPage()) + host, _ := url.Parse(ts.URL) + current := datastore.Rule{Domain: host.Host, Content: "div.copy", Enabled: true} + client := &mocks.OpenAIClientMock{CreateChatCompletionFunc: sequence(jsonSelector("div.copy"), jsonSelector("div.copy p:first-child"))} + rules := rulesStore(current) + + res, err := newSuggester(client, rules).SuggestRule(context.Background(), ts.URL+"/article") + require.NoError(t, err) + assert.Equal(t, "div.copy p:first-child", res.Selector) + assert.Contains(t, client.CreateChatCompletionCalls()[1].Request.Messages[1].Content, "same text as the current result") + assert.Contains(t, client.CreateChatCompletionCalls()[0].Request.Messages[1].Content, "The text extracted now is reported as wrong") + }) + + t.Run("all attempts rejected", func(t *testing.T) { + ts := suggestServer(t, storyPage()) + client := &mocks.OpenAIClientMock{CreateChatCompletionFunc: answer(jsonSelector("div.nothing"))} + rules := rulesStore() + + _, err := newSuggester(client, rules).SuggestRule(context.Background(), ts.URL+"/article") + assert.ErrorIs(t, err, ErrNoSelector) + assert.Len(t, client.CreateChatCompletionCalls(), 3) + assert.Empty(t, rules.SaveIfAbsentCalls()) + }) + + t.Run("existing rule is not replaced", func(t *testing.T) { + ts := suggestServer(t, storyPage()) + host, _ := url.Parse(ts.URL) + for _, enabled := range []bool{true, false} { + t.Run(fmt.Sprintf("enabled=%v", enabled), func(t *testing.T) { + existing := datastore.Rule{Domain: host.Host, Content: "div.thread", Enabled: enabled} + client := &mocks.OpenAIClientMock{CreateChatCompletionFunc: answer(jsonSelector("div.copy"))} + rules := rulesStore(existing) + + res, err := newSuggester(client, rules).SuggestRule(context.Background(), ts.URL+"/article") + require.NoError(t, err) + assert.Equal(t, "div.copy", res.Selector) + assert.False(t, res.Saved) + assert.Len(t, rules.SaveIfAbsentCalls(), 1, "the rule is offered to the store, which keeps the existing one") + assert.Empty(t, rules.SaveCalls(), "Save would replace the rule of the domain") + }) + } + }) + + t.Run("save failure still returns the suggestion", func(t *testing.T) { + ts := suggestServer(t, storyPage()) + client := &mocks.OpenAIClientMock{CreateChatCompletionFunc: answer(jsonSelector("div.copy"))} + rules := rulesStore() + rules.SaveIfAbsentFunc = func(context.Context, datastore.Rule) (bool, error) { return false, errors.New("db down") } + + res, err := newSuggester(client, rules).SuggestRule(context.Background(), ts.URL+"/article") + require.NoError(t, err) + assert.Equal(t, "div.copy", res.Selector) + assert.False(t, res.Saved) + }) + + t.Run("api error is returned without retry", func(t *testing.T) { + ts := suggestServer(t, storyPage()) + client := &mocks.OpenAIClientMock{CreateChatCompletionFunc: func(context.Context, openai.ChatCompletionRequest) (openai.ChatCompletionResponse, error) { + return openai.ChatCompletionResponse{}, errors.New("401 unauthorized") + }} + rules := rulesStore() + + _, err := newSuggester(client, rules).SuggestRule(context.Background(), ts.URL+"/article") + require.ErrorContains(t, err, "401 unauthorized") + assert.NotErrorIs(t, err, ErrNoSelector) + assert.Len(t, client.CreateChatCompletionCalls(), 1) + assert.Empty(t, rules.SaveIfAbsentCalls()) + }) + + t.Run("no choices counts as a bad reply", func(t *testing.T) { + ts := suggestServer(t, storyPage()) + client := &mocks.OpenAIClientMock{CreateChatCompletionFunc: func(context.Context, openai.ChatCompletionRequest) (openai.ChatCompletionResponse, error) { + return openai.ChatCompletionResponse{}, nil + }} + _, err := newSuggester(client, rulesStore()).SuggestRule(context.Background(), ts.URL+"/article") + assert.ErrorIs(t, err, ErrNoSelector) + assert.Len(t, client.CreateChatCompletionCalls(), 3) + }) + + t.Run("page that can't be fetched never reaches the model", func(t *testing.T) { + ts := httptest.NewServer(http.NotFoundHandler()) + unreachable := ts.URL + ts.Close() + client := &mocks.OpenAIClientMock{CreateChatCompletionFunc: answer(jsonSelector("div.copy"))} + _, err := newSuggester(client, rulesStore()).SuggestRule(context.Background(), unreachable+"/article") + require.Error(t, err) + assert.Empty(t, client.CreateChatCompletionCalls()) + }) +} diff --git a/extractor/mocks/rules.go b/extractor/mocks/rules.go index aeb39f71..969c8deb 100644 --- a/extractor/mocks/rules.go +++ b/extractor/mocks/rules.go @@ -32,6 +32,9 @@ import ( // SaveFunc: func(ctx context.Context, rule datastore.Rule) (datastore.Rule, error) { // panic("mock out the Save method") // }, +// SaveIfAbsentFunc: func(ctx context.Context, rule datastore.Rule) (bool, error) { +// panic("mock out the SaveIfAbsent method") +// }, // } // // // use mockedRules in code that requires extractor.Rules @@ -54,6 +57,9 @@ type RulesMock struct { // SaveFunc mocks the Save method. SaveFunc func(ctx context.Context, rule datastore.Rule) (datastore.Rule, error) + // SaveIfAbsentFunc mocks the SaveIfAbsent method. + SaveIfAbsentFunc func(ctx context.Context, rule datastore.Rule) (bool, error) + // calls tracks calls to the methods. calls struct { // All holds details about calls to the All method. @@ -89,12 +95,20 @@ type RulesMock struct { // Rule is the rule argument value. Rule datastore.Rule } + // SaveIfAbsent holds details about calls to the SaveIfAbsent method. + SaveIfAbsent []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // Rule is the rule argument value. + Rule datastore.Rule + } } - lockAll sync.RWMutex - lockDisable sync.RWMutex - lockGet sync.RWMutex - lockGetByID sync.RWMutex - lockSave sync.RWMutex + lockAll sync.RWMutex + lockDisable sync.RWMutex + lockGet sync.RWMutex + lockGetByID sync.RWMutex + lockSave sync.RWMutex + lockSaveIfAbsent sync.RWMutex } // All calls AllFunc. @@ -272,3 +286,39 @@ func (mock *RulesMock) SaveCalls() []struct { mock.lockSave.RUnlock() return calls } + +// SaveIfAbsent calls SaveIfAbsentFunc. +func (mock *RulesMock) SaveIfAbsent(ctx context.Context, rule datastore.Rule) (bool, error) { + if mock.SaveIfAbsentFunc == nil { + panic("RulesMock.SaveIfAbsentFunc: method is nil but Rules.SaveIfAbsent was just called") + } + callInfo := struct { + Ctx context.Context + Rule datastore.Rule + }{ + Ctx: ctx, + Rule: rule, + } + mock.lockSaveIfAbsent.Lock() + mock.calls.SaveIfAbsent = append(mock.calls.SaveIfAbsent, callInfo) + mock.lockSaveIfAbsent.Unlock() + return mock.SaveIfAbsentFunc(ctx, rule) +} + +// SaveIfAbsentCalls gets all the calls that were made to SaveIfAbsent. +// Check the length with: +// +// len(mockedRules.SaveIfAbsentCalls()) +func (mock *RulesMock) SaveIfAbsentCalls() []struct { + Ctx context.Context + Rule datastore.Rule +} { + var calls []struct { + Ctx context.Context + Rule datastore.Rule + } + mock.lockSaveIfAbsent.RLock() + calls = mock.calls.SaveIfAbsent + mock.lockSaveIfAbsent.RUnlock() + return calls +} diff --git a/extractor/outline.go b/extractor/outline.go new file mode 100644 index 00000000..4e55250d --- /dev/null +++ b/extractor/outline.go @@ -0,0 +1,174 @@ +package extractor + +import ( + "fmt" + "strings" + + "golang.org/x/net/html" + "golang.org/x/net/html/atom" +) + +const ( + outlineMinText = 100 // elements with less visible text are left out of the outline + outlineMaxLines = 120 + outlineMaxDepth = 12 + outlineMaxClass = 3 + outlineMaxName = 40 + outlineSnippet = 80 + outlineMaxThresh = 1 << 20 +) + +// outlineNode is an element of the page body with the size of its visible text +type outlineNode struct { + node *html.Node + textLen int + paragraphs int + children []*outlineNode +} + +// pageOutline describes the structure of the page body for a model: one line per element that holds +// a meaningful amount of text, with its tag, id, classes, text length and paragraph count. +// The outline is much shorter than the raw HTML and leaves out scripts, styles and markup noise. +func pageOutline(root *html.Node) string { + body := findBody(root) + if body == nil { + return "" + } + tree := buildOutline(body) + + // raise the text threshold until the outline fits + var lines []string + for threshold := outlineMinText; threshold <= outlineMaxThresh; threshold *= 2 { + lines = lines[:0] + tree.render(&lines, 0, threshold) + if len(lines) <= outlineMaxLines { + break + } + } + return strings.Join(lines, "\n") +} + +// visibleTextLen is the number of characters of visible text under n, whitespace collapsed +func visibleTextLen(n *html.Node) int { + if n == nil { + return 0 + } + return buildOutline(n).textLen +} + +func findBody(n *html.Node) *html.Node { + if n.Type == html.ElementNode && n.DataAtom == atom.Body { + return n + } + for c := n.FirstChild; c != nil; c = c.NextSibling { + if b := findBody(c); b != nil { + return b + } + } + return nil +} + +func skipped(n *html.Node) bool { + switch n.DataAtom { + case atom.Script, atom.Style, atom.Noscript, atom.Template, atom.Svg, atom.Iframe: + return true + } + return false +} + +func buildOutline(n *html.Node) *outlineNode { + o := &outlineNode{node: n} + if n.DataAtom == atom.P { + o.paragraphs = 1 + } + for c := n.FirstChild; c != nil; c = c.NextSibling { + switch { + case c.Type == html.TextNode: + o.textLen += len([]rune(strings.Join(strings.Fields(c.Data), " "))) + case c.Type == html.ElementNode && !skipped(c): + child := buildOutline(c) + o.textLen += child.textLen + o.paragraphs += child.paragraphs + o.children = append(o.children, child) + default: // comments, doctype and skipped elements carry no visible text + } + } + return o +} + +func (o *outlineNode) render(lines *[]string, depth, threshold int) { + if o.textLen < threshold || depth > outlineMaxDepth { + return + } + line := fmt.Sprintf("%s%s text=%d p=%d", strings.Repeat(" ", depth), o.label(), o.textLen, o.paragraphs) + *lines = append(*lines, line) + before := len(*lines) + for _, c := range o.children { + c.render(lines, depth+1, threshold) + } + if len(*lines) == before { // nothing below, show how the text starts + (*lines)[before-1] += fmt.Sprintf(" %q", firstText(o.node, outlineSnippet)) + } +} + +// label is tag#id.class.class, with overlong names cut +func (o *outlineNode) label() string { + var id string + var classes []string + for _, a := range o.node.Attr { + switch a.Key { + case "id": + id = cutName(a.Val) + case "class": + for _, c := range strings.Fields(a.Val) { + if len(classes) < outlineMaxClass { + classes = append(classes, cutName(c)) + } + } + default: // other attributes do not help to find the article + } + } + label := o.node.Data + if id != "" { + label += "#" + id + } + if len(classes) > 0 { + label += "." + strings.Join(classes, ".") + } + return label +} + +func cutName(s string) string { + s = strings.Join(strings.Fields(s), "") + if r := []rune(s); len(r) > outlineMaxName { + return string(r[:outlineMaxName]) + } + return s +} + +// firstText returns up to limit characters of the visible text under n +func firstText(n *html.Node, limit int) string { + var parts []string + var size int + var walk func(*html.Node) + walk = func(n *html.Node) { + for c := n.FirstChild; c != nil && size < limit; c = c.NextSibling { + switch { + case c.Type == html.TextNode: + if t := strings.Join(strings.Fields(c.Data), " "); t != "" { + parts = append(parts, t) + size += len([]rune(t)) + 1 + } + case c.Type == html.ElementNode && !skipped(c): + walk(c) + default: // comments, doctype and skipped elements carry no visible text + } + } + } + walk(n) + text := strings.Join(parts, " ") + if r := []rune(text); len(r) > limit { + return string(r[:limit]) + } + return text +} diff --git a/extractor/outline_test.go b/extractor/outline_test.go new file mode 100644 index 00000000..4af3cbed --- /dev/null +++ b/extractor/outline_test.go @@ -0,0 +1,72 @@ +package extractor + +import ( + "fmt" + "strings" + "testing" + + "github.com/PuerkitoBio/goquery" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/net/html" +) + +func outlineOf(t *testing.T, page string) string { + t.Helper() + doc, err := goquery.NewDocumentFromReader(strings.NewReader(page)) + require.NoError(t, err) + return pageOutline(doc.Nodes[0]) +} + +func TestPageOutline(t *testing.T) { + long := strings.Repeat("word ", 40) // 199 characters once the trailing space is collapsed + short := strings.Repeat("word ", 15) + + t.Run("shows structure, sizes and the start of the text", func(t *testing.T) { + page := `t + +
+ +

First paragraph ` + short + `

Second ` + short + `

+
` + out := outlineOf(t, page) + lines := strings.Split(out, "\n") + require.Len(t, lines, 3, out) + assert.Regexp(t, `^body text=\d+ p=2$`, lines[0]) + assert.Regexp(t, `^ div#main\.post\.wide text=\d+ p=2$`, lines[1]) + assert.Regexp(t, `^ article\.body text=\d+ p=2 "First paragraph word word`, lines[2]) + assert.NotContains(t, out, "nav", "elements with little text are left out") + assert.NotContains(t, out, "secret") + assert.NotContains(t, out, "inline") + }) + + t.Run("leaves out scripts, styles and svg when counting text", func(t *testing.T) { + page := `

` + long + `

` + + `` + strings.Repeat("y", 5000) + `
` + assert.Regexp(t, `div#a text=199 p=1`, outlineOf(t, page)) + }) + + t.Run("long names are cut and at most three classes are shown", func(t *testing.T) { + page := `
` + long + `
` + out := outlineOf(t, page) + assert.Contains(t, out, "div#"+strings.Repeat("i", 40)+".a.b.c ") + assert.NotContains(t, out, ".d") + }) + + t.Run("raises the threshold until the outline fits", func(t *testing.T) { + var sb strings.Builder + sb.WriteString("
") + for i := range 300 { + fmt.Fprintf(&sb, `
%s
`, i, strings.Repeat("a", 150)) + } + sb.WriteString(`
` + strings.Repeat("b", 5000) + `
`) + out := outlineOf(t, sb.String()) + assert.LessOrEqual(t, len(strings.Split(out, "\n")), outlineMaxLines) + assert.Contains(t, out, "section#big", "the section holding most of the text survives") + assert.NotContains(t, out, "section#s1 ") + }) + + t.Run("no body gives nothing to show", func(t *testing.T) { + assert.Empty(t, pageOutline(&html.Node{Type: html.DocumentNode})) + }) +} diff --git a/extractor/readability.go b/extractor/readability.go index 3e128e15..2c930b2f 100644 --- a/extractor/readability.go +++ b/extractor/readability.go @@ -26,6 +26,7 @@ type Rules interface { Get(ctx context.Context, rURL string) (datastore.Rule, bool) GetByID(ctx context.Context, id bson.ObjectID) (datastore.Rule, bool) Save(ctx context.Context, rule datastore.Rule) (datastore.Rule, error) + SaveIfAbsent(ctx context.Context, rule datastore.Rule) (bool, error) Disable(ctx context.Context, id bson.ObjectID) error All(ctx context.Context) []datastore.Rule } @@ -115,10 +116,9 @@ func (f *UReadability) ExtractByRule(ctx context.Context, reqURL string, rule *d return f.extractWithRules(ctx, reqURL, rule) } -// ExtractWithRules is the core function that handles extraction with or without a specific rule +// extractWithRules is the core function that handles extraction with or without a specific rule func (f *UReadability) extractWithRules(ctx context.Context, reqURL string, rule *datastore.Rule) (*Response, error) { log.Printf("[INFO] extract %s", reqURL) - rb := &Response{} // look up a rule by domain once up front (unless one was explicitly passed) so retriever // selection and getContent share the same lookup instead of paying for two round-trips. @@ -128,15 +128,27 @@ func (f *UReadability) extractWithRules(ctx context.Context, reqURL string, rule } } - result, err := f.pickRetriever(rule).Retrieve(ctx, reqURL) + rb, body, err := f.fetch(ctx, reqURL, rule) if err != nil { return nil, err } + return f.parse(ctx, reqURL, body, rb, rule) +} - rb.URL = result.URL - - var body string +// fetch retrieves the page and converts it to utf8, returning a Response with the final URL, content type and charset set +func (f *UReadability) fetch(ctx context.Context, reqURL string, rule *datastore.Rule) (rb *Response, body string, err error) { + result, err := f.pickRetriever(rule).Retrieve(ctx, reqURL) + if err != nil { + return nil, "", err + } + rb = &Response{URL: result.URL} rb.ContentType, rb.Charset, body = f.toUtf8(result.Body, result.Header) + return rb, body, nil +} + +// parse extracts the article from the fetched body into rb, using the rule when it is given +func (f *UReadability) parse(ctx context.Context, reqURL, body string, rb *Response, rule *datastore.Rule) (*Response, error) { + var err error rb.Content, rb.Rich, err = f.getContent(ctx, body, reqURL, rule) if err != nil { log.Printf("[WARN] failed to parse %s, error=%v", reqURL, err) @@ -173,6 +185,21 @@ func (f *UReadability) extractWithRules(ctx context.Context, reqURL string, rule return rb, nil } +// extractWithSelector returns the inner HTML of every element matching the selector, concatenated +func extractWithSelector(body, selector string) (string, error) { + doc, err := goquery.NewDocumentFromReader(strings.NewReader(body)) + if err != nil { + return "", err + } + var res string + doc.Find(selector).Each(func(_ int, s *goquery.Selection) { + if html, err := s.Html(); err == nil { + res += html + } + }) + return res, nil +} + // getContent retrieves content from raw body string, both content (text only) and rich (with html tags). // if rule is provided, it tries the custom rule first and falls back to the general parser on failure. // rule lookup for a given URL is done upstream in extractWithRules. @@ -190,16 +217,10 @@ func (f *UReadability) getContent(_ context.Context, body, reqURL string, rule * // custom rules parser customParser := func(body, reqURL string, rule datastore.Rule) (content, rich string, err error) { log.Printf("[DEBUG] custom extractor for %s", reqURL) - dbody, err := goquery.NewDocumentFromReader(strings.NewReader(body)) + res, err := extractWithSelector(body, rule.Content) if err != nil { return "", "", err } - var res string - dbody.Find(rule.Content).Each(func(_ int, s *goquery.Selection) { - if html, err := s.Html(); err == nil { - res += html - } - }) if res == "" { return "", "", fmt.Errorf("nothing extracted from %s, rule=%v", reqURL, rule) } diff --git a/extractor/summary.go b/extractor/summary.go index 2ae8fd30..e74870f3 100644 --- a/extractor/summary.go +++ b/extractor/summary.go @@ -97,8 +97,8 @@ func (f *UReadability) GenerateSummary(ctx context.Context, content string) (str func summaryID(model, prompt, content string) string { h := sha256.New() for _, s := range []string{model, prompt, content} { - h.Write([]byte(s)) - h.Write([]byte{0}) + _, _ = h.Write([]byte(s)) + _, _ = h.Write([]byte{0}) } return hex.EncodeToString(h.Sum(nil)) } diff --git a/rest/server.go b/rest/server.go index e4680f74..34243e48 100644 --- a/rest/server.go +++ b/rest/server.go @@ -4,6 +4,7 @@ package rest import ( "context" "crypto/subtle" + "errors" "fmt" "html/template" "net/http" @@ -25,9 +26,9 @@ import ( const writeTimeout = 150 * time.Second -// summaryBudget caps extraction plus summaries per request, so the response is written +// openAIBudget caps extraction plus OpenAI calls per request, so the response is written // before writeTimeout drops the connection; a var so tests can shorten it -var summaryBudget = writeTimeout - 10*time.Second +var openAIBudget = writeTimeout - 10*time.Second // Server is a basic rest server providing access to store and invoking parser type Server struct { @@ -59,8 +60,8 @@ func (s *Server) Run(ctx context.Context, address string, port int, frontendDir // are the only potentially long-running handlers (other handlers — static files, rule // CRUD, /ping — finish in milliseconds and are unaffected by this ceiling). 150s covers // the worst-case Cloudflare path: 1 initial request + 2 retries with 11s/22s exponential - // backoff + up to 30s per CF request. OpenAI summaries get only what remains of - // summaryBudget after extraction. If extraction ever moves off the server-wide + // backoff + up to 30s per CF request. OpenAI calls get only what remains of + // openAIBudget after extraction. If extraction ever moves off the server-wide // timeout, wrap only those routes with http.TimeoutHandler instead. WriteTimeout: writeTimeout, IdleTimeout: 30 * time.Second, @@ -98,6 +99,7 @@ func (s *Server) routes(frontendDir string) http.Handler { protectedGroup.HandleFunc("POST /rule", s.saveRule) protectedGroup.HandleFunc("POST /toggle-rule/{id}", s.toggleRule) protectedGroup.HandleFunc("POST /preview", s.handlePreview) + protectedGroup.HandleFunc("POST /content-parsed-wrong", s.contentParsedWrong) }) }) @@ -221,7 +223,7 @@ func (s *Server) extractArticleEmulateReadability(w http.ResponseWriter, r *http } if summary { - ctx, cancel := context.WithDeadline(r.Context(), start.Add(summaryBudget)) + ctx, cancel := context.WithDeadline(r.Context(), start.Add(openAIBudget)) defer cancel() if res.Summary, err = s.Readability.GenerateSummary(ctx, res.Content); err != nil { rest.SendErrorJSON(w, r, log.Default(), http.StatusInternalServerError, err, "can't generate summary") @@ -232,6 +234,32 @@ func (s *Server) extractArticleEmulateReadability(w http.ResponseWriter, r *http rest.RenderJSON(w, &res) } +// contentParsedWrong asks OpenAI for a selector of the article on a page the parser got wrong. +// The selector is stored as a disabled rule when the domain has none; it is always returned with +// the page extracted by it, so it can be tried in the rule preview before it is enabled. +func (s *Server) contentParsedWrong(w http.ResponseWriter, r *http.Request) { + start := time.Now() + extractURL := r.URL.Query().Get("url") + if extractURL == "" { + rest.SendErrorJSON(w, r, log.Default(), http.StatusBadRequest, nil, "url parameter is required") + return + } + + ctx, cancel := context.WithDeadline(r.Context(), start.Add(openAIBudget)) + defer cancel() + res, err := s.Readability.SuggestRule(ctx, extractURL) + switch { + case errors.Is(err, extractor.ErrOpenAIDisabled): + rest.SendErrorJSON(w, r, log.Default(), http.StatusBadRequest, nil, "OpenAI key is not set") + case errors.Is(err, extractor.ErrNoSelector): + rest.SendErrorJSON(w, r, log.Default(), http.StatusUnprocessableEntity, err, "can't find a selector for this page") + case err != nil: + rest.SendErrorJSON(w, r, log.Default(), http.StatusInternalServerError, err, "can't suggest a rule") + default: + rest.RenderJSON(w, res) + } +} + // generates previews for the provided test URLs func (s *Server) handlePreview(w http.ResponseWriter, r *http.Request) { start := time.Now() @@ -268,7 +296,7 @@ func (s *Server) handlePreview(w http.ResponseWriter, r *http.Request) { // summaries run after every extraction so they only spend what is left of the budget; // once it is gone, the remaining previews render without a summary - summaryCtx, cancel := context.WithDeadline(r.Context(), start.Add(summaryBudget)) + summaryCtx, cancel := context.WithDeadline(r.Context(), start.Add(openAIBudget)) defer cancel() for i := range responses { if s.Readability.OpenAI == nil || summaryCtx.Err() != nil { diff --git a/rest/server_test.go b/rest/server_test.go index 9fd4efd8..fddb4d8f 100644 --- a/rest/server_test.go +++ b/rest/server_test.go @@ -3,6 +3,7 @@ package rest import ( "context" "encoding/json" + "errors" "fmt" "html/template" "io" @@ -714,9 +715,9 @@ func TestServer_ExtractWithSummary(t *testing.T) { } func TestServer_SummaryBudget(t *testing.T) { - orig := summaryBudget - summaryBudget = 200 * time.Millisecond - t.Cleanup(func() { summaryBudget = orig }) + orig := openAIBudget + openAIBudget = 200 * time.Millisecond + t.Cleanup(func() { openAIBudget = orig }) // a model that answers only after 2s, or gives up when its context ends slowModel := func(ctx context.Context, _ openai.ChatCompletionRequest) (openai.ChatCompletionResponse, error) { @@ -786,6 +787,124 @@ func TestServer_SummaryBudget(t *testing.T) { }) } +func TestServer_ContentParsedWrong(t *testing.T) { + article := strings.Repeat("The quick brown fox jumps over the lazy dog. ", 12) + thread := strings.Repeat("Well, I think so, but then again, who knows, maybe not, and yet here we are, arguing about it, again. ", 6) + page := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `Story
  • Home
+

%[1]s

%[1]s

%[1]s

%[2]s

%[2]s

%[2]s

%[2]s

`, + article, thread) + })) + defer page.Close() + + suggest := func(t *testing.T, ts *httptest.Server, query string) (status int, body string) { + t.Helper() + resp, err := post(t, ts.URL+"/api/content-parsed-wrong"+query, "") + require.NoError(t, err) + b, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + return resp.StatusCode, string(b) + } + + t.Run("requires credentials", func(t *testing.T) { + ts, srv := startupT(t) + defer ts.Close() + client := &mocks.OpenAIClientMock{CreateChatCompletionFunc: summaryAnswer(`{"selector": "div.copy"}`)} + srv.Readability.OpenAI = client + + resp, err := http.Post(ts.URL+"/api/content-parsed-wrong?url="+page.URL, "", http.NoBody) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) + assert.Empty(t, client.CreateChatCompletionCalls()) + }) + + t.Run("url is required", func(t *testing.T) { + ts, srv := startupT(t) + defer ts.Close() + srv.Readability.OpenAI = &mocks.OpenAIClientMock{CreateChatCompletionFunc: summaryAnswer(`{"selector": "div.copy"}`)} + status, body := suggest(t, ts, "") + assert.Equal(t, http.StatusBadRequest, status, body) + assert.Contains(t, body, "url parameter is required") + }) + + t.Run("no OpenAI client", func(t *testing.T) { + ts, _ := startupT(t) + defer ts.Close() + status, body := suggest(t, ts, "?url="+page.URL) + assert.Equal(t, http.StatusBadRequest, status, body) + assert.Contains(t, body, "OpenAI key is not set") + }) + + t.Run("suggestion is returned and stored as a disabled rule", func(t *testing.T) { + ts, srv := startupT(t) + defer ts.Close() + srv.Readability.OpenAI = &mocks.OpenAIClientMock{CreateChatCompletionFunc: summaryAnswer(`{"selector": "div.copy"}`)} + + status, body := suggest(t, ts, "?url="+page.URL) + require.Equal(t, http.StatusOK, status, body) + var res extractor.Suggestion + require.NoError(t, json.Unmarshal([]byte(body), &res)) + assert.Equal(t, "div.copy", res.Selector) + assert.True(t, res.Saved) + assert.Contains(t, res.Response.Content, "The quick brown fox") + assert.NotContains(t, res.Response.Content, "Well, I think so") + + rules := srv.Readability.Rules.All(context.Background()) + require.Len(t, rules, 1) + assert.Equal(t, "div.copy", rules[0].Content) + assert.False(t, rules[0].Enabled) + _, found := srv.Readability.Rules.Get(context.Background(), page.URL) + assert.False(t, found, "readers must not get the rule before it is enabled") + }) + + t.Run("no usable selector", func(t *testing.T) { + ts, srv := startupT(t) + defer ts.Close() + srv.Readability.OpenAI = &mocks.OpenAIClientMock{CreateChatCompletionFunc: summaryAnswer(`{"selector": "div.nothing"}`)} + status, body := suggest(t, ts, "?url="+page.URL) + assert.Equal(t, http.StatusUnprocessableEntity, status, body) + assert.Empty(t, srv.Readability.Rules.All(context.Background())) + }) + + t.Run("model error", func(t *testing.T) { + ts, srv := startupT(t) + defer ts.Close() + srv.Readability.OpenAI = &mocks.OpenAIClientMock{CreateChatCompletionFunc: func(context.Context, openai.ChatCompletionRequest) (openai.ChatCompletionResponse, error) { + return openai.ChatCompletionResponse{}, errors.New("401 unauthorized") + }} + status, body := suggest(t, ts, "?url="+page.URL) + assert.Equal(t, http.StatusInternalServerError, status, body) + assert.Contains(t, body, "can't suggest a rule") + }) + + t.Run("gives up when the budget runs out", func(t *testing.T) { + orig := openAIBudget + openAIBudget = 200 * time.Millisecond + t.Cleanup(func() { openAIBudget = orig }) + + ts, srv := startupT(t) + defer ts.Close() + client := &mocks.OpenAIClientMock{CreateChatCompletionFunc: func(ctx context.Context, _ openai.ChatCompletionRequest) (openai.ChatCompletionResponse, error) { + select { + case <-ctx.Done(): + return openai.ChatCompletionResponse{}, ctx.Err() + case <-time.After(2 * time.Second): + return summaryAnswer(`{"selector": "div.copy"}`)(ctx, openai.ChatCompletionRequest{}) + } + }} + srv.Readability.OpenAI = client + + st := time.Now() + status, body := suggest(t, ts, "?url="+page.URL) + assert.Less(t, time.Since(st), time.Second) + assert.Equal(t, http.StatusInternalServerError, status, body) + assert.Len(t, client.CreateChatCompletionCalls(), 1, "no further attempts once the budget is gone") + assert.Empty(t, srv.Readability.Rules.All(context.Background())) + }) +} + func summaryAnswer(text string) func(context.Context, openai.ChatCompletionRequest) (openai.ChatCompletionResponse, error) { return func(context.Context, openai.ChatCompletionRequest) (openai.ChatCompletionResponse, error) { return openai.ChatCompletionResponse{Choices: []openai.ChatCompletionChoice{{Message: openai.ChatCompletionMessage{Content: text}}}}, nil @@ -897,6 +1016,18 @@ func newRulesStoreMock() *mocks.RulesMock { rules = append(rules, rule) return rule, nil }, + SaveIfAbsentFunc: func(_ context.Context, rule datastore.Rule) (bool, error) { + mu.Lock() + defer mu.Unlock() + for _, r := range rules { + if r.Domain == rule.Domain { + return false, nil + } + } + rule.ID = bson.NewObjectID() + rules = append(rules, rule) + return true, nil + }, DisableFunc: func(_ context.Context, id bson.ObjectID) error { mu.Lock() defer mu.Unlock()