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

Issue 68: Implement redirect handling. #70

Closed
wants to merge 2 commits into from

Conversation

kaladay
Copy link
Contributor

@kaladay kaladay commented Jan 30, 2024

resolves #68

Perform redirects when receiving HTTP 301 and HTTP 302 status codes. This implements a hard-coded max redirect of 10.
A custom property could be implemented but this is left for a future decision or for a reviewers input.

This adds new parameters to the RestService HTTP functions for performing the redirets. The functions are all made to have the same parameters so that the redirect can pass a consistent callback. Unused parameters, such as json in the get() method, are ignored.

Logging is added for redirects, when logLevel is set to DEBUG (case insensitive). An error is printed when max redirects is reached.

When no redirection is requested, the redirectRecurse() function returns false so that the caller can continue normal behavior.

Perform redirects when receiving HTTP 301 and HTTP 302 status codes.
This implements a hard-coded max redirect of 10.
A custom property could be implemented but this is left for a future decision or for a reviewers input.

This adds new parameters to the RestService HTTP functions for performing the redirets.
The functions are all made to have the same parameters so that the redirect can pass a consistent callback.
Unused parameters, such as `json` in the `get()` method, are ignored.

Logging is added for redirects, when `logLevel` is set to `DEBUG` (case insensitive).
An error is printed when max redirects is reached.

When no redirection is requested, the `redirectRecurse()` function returns false so that the caller can continue normal behavior.
Make the code less redundant by adding an additional private method.
@@ -8,14 +8,15 @@ export class RestService {
return request(req, cb);
}

public get(url: string, contentType: string = 'application/json'): Promise<any> {
public get(url: string, json: any = null, contentType: string = 'application/json', accept: string | string[] = [ 'application/json', 'text/plain' ], redirectCount: number = 0): Promise<any> {
Copy link
Contributor

@wwelling wwelling Jan 30, 2024

Choose a reason for hiding this comment

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

For code cleanliness, methods should have two or less arguments. Dyadic is preferred max number of arguments. Triadic is borderline messy. Four or more is in need of code cleanup.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer a type or interface with these arguments as properties opposed to adding too many arguments.

Copy link
Contributor

@wwelling wwelling left a comment

Choose a reason for hiding this comment

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

Have you tried using followRedirect, followAllRedirect, and/or maxRedirects of the request library?

Also, noting we need to replace the use of request as it is deprecated. Somehow this has went unnoticed.

@wwelling wwelling requested a review from rmathew1011 January 31, 2024 21:40
@kaladay
Copy link
Contributor Author

kaladay commented Jan 31, 2024

Have you tried using followRedirect, followAllRedirect, and/or maxRedirects of the request library?

Also, noting we need to replace the use of request as it is deprecated. Somehow this has went unnoticed.

It is hardcoded to not follow redirects on POST, PUT, etc...

Looks like we need followAllRedirects.

@kaladay
Copy link
Contributor Author

kaladay commented Feb 1, 2024

I found problems and the followAllRedirects does not work correctly.
Updated the issue with additional information and I am closing this PR in favor of some future better implementation

@kaladay kaladay closed this Feb 1, 2024
@kaladay kaladay deleted the 68-moved_permanently branch February 1, 2024 22:34
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.

Fix "HTTP/1.1 301 Moved Permanently" failures and replaced deprecated modules.
3 participants