Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add link-preview handler #929

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from
Open

Conversation

yinebebt
Copy link

add link-preview handler

@yinebebt yinebebt mentioned this pull request Nov 28, 2024
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/main.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
@or-else
Copy link
Contributor

or-else commented Nov 28, 2024

  1. The service is open for abuse: anyone on the internet, authorized or not, can make requests to this service. Please add authentication.
  2. There is no cache. I guess it's OK for now, but cache should be added at some point.

Also please see comments in the code.

Thank you.

@or-else

This comment was marked as resolved.

server/main.go Outdated Show resolved Hide resolved
server/main.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Show resolved Hide resolved
server/linkpreview.go Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Outdated Show resolved Hide resolved
server/linkpreview.go Show resolved Hide resolved
server/linkpreview.go Show resolved Hide resolved
data := strings.ToLower(token.Data)
if data == "meta" {
var name, property, content string
for _, attr := range token.Attr {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use tokenizer.TagAttr() instead of iterating attributes directly.

Copy link
Contributor

@or-else or-else Nov 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve my previous comment on this.

server/linkpreview.go Outdated Show resolved Hide resolved
data := strings.ToLower(token.Data)
if data == "meta" {
var name, property, content string
for _, attr := range token.Attr {
Copy link
Contributor

@or-else or-else Nov 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve my previous comment on this.

preview.Title = strings.TrimSpace(tokenizer.Token().Data)
}
case html.EndTagToken:
if tokenizer.Token().DataAtom == atom.Title {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to test it. Just assign false without testing. Consider this example:

<body>
<head>
<title><!-- no closing title -->
</head>
<body>This is not a title</body>
</html>

}

case html.TextToken:
if preview.Title == "" && inTitleTag {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if inTitleTag {
    if preview.Title == "" {
         preview.Title = strings.TrimSpace(tokenizer.Token().Data)
    }
    inTitleTag = false
}

}
}

if strings.HasPrefix(property, "og:") && content != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if content != "" {
    if strings.HasPrefix(property, "og:") {
        ...
    } else if name == "description" && preview.Description == "" {
        ...
    }
}

or

if content == "" {
    continue
}


case html.TextToken:
if preview.Title == "" && inTitleTag {
preview.Title = strings.TrimSpace(tokenizer.Token().Data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving strings.TrimSpace to sanitizePreview and applying it to all fields.

w.WriteHeader(http.StatusOK)
return
}
w.Header().Set("Content-Type", "application/json")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this up, before checking for MethodHead. Response to HEAD should have "Content-Type" header.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants