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

fix(client): Return nil and error if endpoint is empty string #2773

Merged
merged 1 commit into from
Aug 18, 2024

Conversation

akosiaris
Copy link
Contributor

@akosiaris akosiaris commented Jul 30, 2024

Why:
Passing an empty string as an endpoint to Client when instantiating a
new client might seem like something that should never happen but I
managed to trigger it while parsing some input files to register feeds
in bulk.

What:
In the execute() function, check early if the endpoint is "" and then
return immediately nil and a new error, named ErrWrongEndpoint with a
descriptive string

Do you follow the guidelines?

Copy link
Member

@fguillot fguillot left a comment

Choose a reason for hiding this comment

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

Instead of returning nil, the function execute can return an error if the endpoint is an empty string.

v2/client/request.go

Lines 64 to 72 in 569529d

func (r *request) execute(method, path string, data interface{}) (io.ReadCloser, error) {
if r.endpoint[len(r.endpoint)-1:] == "/" {
r.endpoint = r.endpoint[:len(r.endpoint)-1]
}
u, err := url.Parse(r.endpoint + path)
if err != nil {
return nil, err
}

A new error type can be exposed outside the package for this purpose:

v2/client/request.go

Lines 23 to 30 in 569529d

// List of exposed errors.
var (
ErrNotAuthorized = errors.New("miniflux: unauthorized (bad credentials)")
ErrForbidden = errors.New("miniflux: access forbidden")
ErrServerError = errors.New("miniflux: internal server error")
ErrNotFound = errors.New("miniflux: resource not found")
ErrBadRequest = errors.New("miniflux: bad request")
)

@akosiaris
Copy link
Contributor Author

Taking a quick look, I think your solution is better than mine. As long as no Client's endpoint field is used outside of a request context, it shoud work fine.

I 'll try and code it, run it for a few days and then get back to you.

Thanks for the suggestion!

@akosiaris akosiaris force-pushed the nil_client branch 4 times, most recently from 629bec6 to da64ee5 Compare August 17, 2024 22:53
@akosiaris akosiaris changed the title fix(client): Return nil if endpoint is empty string fix(client): Return nil and error if endpoint is empty string Aug 17, 2024
@akosiaris
Copy link
Contributor Author

Updated per your suggestion. I came up with the error name and string but happy to alter name and wording.

client/request.go Outdated Show resolved Hide resolved
client/request.go Outdated Show resolved Hide resolved
Why:
Passing an empty string as an endpoint to Client when instantiating a
new client might seem like something that should never happen but I
managed to trigger it while parsing some input files to register feeds
in bulk.

What:
In the execute() function, check early if the endpoint is "" and then
return immediately nil and a new error, named ErrEmptyEndpoint with a
descriptive string
@fguillot fguillot merged commit 89ff33d into miniflux:main Aug 18, 2024
16 checks passed
@akosiaris akosiaris deleted the nil_client branch September 7, 2024 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants