Skip to content

Add FCH #139

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add FCH #139

wants to merge 1 commit into from

Conversation

a-thieme
Copy link
Collaborator

No description provided.

@a-thieme a-thieme requested a review from pulsejet April 20, 2025 17:36
@a-thieme a-thieme added enhancement New feature or request std go-ndn issues labels Apr 20, 2025
@zjkmxy zjkmxy requested a review from Copilot April 20, 2025 21:31
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new FCH client for the NDN-FCH service, enabling HTTP queries to retrieve router information.

  • Introduces core types (Request, Response, Router) with helper functions to apply defaults and build query URLs.
  • Implements the Query function to perform HTTP requests and parse responses into routers.
Files not reviewed (1)
  • go.mod: Language not supported
Comments suppressed due to low confidence (1)

std/ndn/fch/fch.go:47

  • The function 'max' is referenced here but is not defined. Consider defining a helper function or using appropriate logic to ensure req.Count is at least 1.
req.Count = max(1, req.Count)

}
hReq.Header.Set("Accept", "application/json, text/plain, */*")

hRes, e := http.DefaultClient.Do(hReq)
Copy link
Preview

Copilot AI Apr 20, 2025

Choose a reason for hiding this comment

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

The HTTP response body is not closed after reading, which could lead to resource leaks. Consider adding 'defer hRes.Body.Close()' after checking for a nil error.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Copy link
Member

Choose a reason for hiding this comment

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

This comment is accurate. By document:

If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close. If the Body is not both read to EOF and closed, the Client's underlying RoundTripper (typically Transport) may not be able to re-use a persistent TCP connection to the server for a subsequent "keep-alive" request.

io.ReadAll does not automatically close it.

Copy link
Member

@zjkmxy zjkmxy left a comment

Choose a reason for hiding this comment

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

Tested with the following code and it works.

res, err := fch.Query(context.Background(), fch.Request{})
if err != nil {
	println(err)
} else {
	retJson, _ := json.Marshal(res)
	println(string(retJson))
}

}
hReq.Header.Set("Accept", "application/json, text/plain, */*")

hRes, e := http.DefaultClient.Do(hReq)
Copy link
Member

Choose a reason for hiding this comment

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

This comment is accurate. By document:

If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close. If the Body is not both read to EOF and closed, the Client's underlying RoundTripper (typically Transport) may not be able to re-use a persistent TCP connection to the server for a subsequent "keep-alive" request.

io.ReadAll does not automatically close it.

"github.com/gorilla/schema"
)

var encoder = schema.NewEncoder()
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I don't like using a global variable.

req.applyDefaults()
u, e := req.toURL()
if e != nil {
return res, e
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to write explicitly the values when using named result parameters.

routers := bytes.Split(body, []byte{','})
for _, router := range routers {
if len(router) == 0 {
return res, errors.New("empty response")
Copy link
Member

Choose a reason for hiding this comment

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

Why do not check if routers array is empty, but to check individual element instead?

return res, errors.New("empty response")
}

connect := string(router)
Copy link
Member

Choose a reason for hiding this comment

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

nit: Connect sounds too much like a function. Maybe rename to uri.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request std go-ndn issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants