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: Lost username when setting new URL with a path (IDFGH-904) #3250

Closed

Conversation

hongquan
Copy link
Contributor

@hongquan hongquan commented Apr 3, 2019

This PR is to fix a bug, in which, when calling esp_http_client_set_url with a path ("/api/"), the username and password set earlier with esp_http_client_init() are lost.

Test cases included. This is the test result:

Press ENTER to see the list of tests.
Running Input Param Tests...
Note: tcpip_adapter_init() has been called. Until next reset, TCP/IP task will periodicially allocate memory and consume CPU time.
E (1898) HTTP_CLIENT: config should have either URL or host & path
/home/quan/Works/Forks/esp-idf/components/esp_http_client/test/test_http_client.c:28:Input Param Tests:PASS
Running Get username...
/home/quan/Works/Forks/esp-idf/components/esp_http_client/test/test_http_client.c:54:Get username:PASS
Running Get password...
/home/quan/Works/Forks/esp-idf/components/esp_http_client/test/test_http_client.c:72:Get password:PASS
Running Username not lost...
/home/quan/Works/Forks/esp-idf/components/esp_http_client/test/test_http_client.c:94:Username not lost:PASS

-----------------------
4 Tests 0 Failures 0 Ignored 
OK

To able to test, I have to add 2 more functions: esp_http_client_get_username and esp_http_client_get_password.

@CLAassistant
Copy link

CLAassistant commented Apr 3, 2019

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot changed the title Fix: Lost username when setting new URL with a path Fix: Lost username when setting new URL with a path (IDFGH-904) Apr 3, 2019
@hongquan
Copy link
Contributor Author

hongquan commented Apr 3, 2019

Add 1 more test case and this is the test result:

Press ENTER to see the list of tests.
Running Input Param Tests...
Note: tcpip_adapter_init() has been called. Until next reset, TCP/IP task will periodicially allocate memory and consume CPU time.
E (1602) HTTP_CLIENT: config should have either URL or host & path
/home/quan/Works/Forks/esp-idf/components/esp_http_client/test/test_http_client.c:28:Input Param Tests:PASS
Running Get username...
/home/quan/Works/Forks/esp-idf/components/esp_http_client/test/test_http_client.c:54:Get username:PASS
Running Get password...
/home/quan/Works/Forks/esp-idf/components/esp_http_client/test/test_http_client.c:72:Get password:PASS
Running Username not lost...
/home/quan/Works/Forks/esp-idf/components/esp_http_client/test/test_http_client.c:94:Username not lost:PASS
Running Username is reset...
D (1722) HTTP_CLIENT: New host assign = httpbin.org
/home/quan/Works/Forks/esp-idf/components/esp_http_client/test/test_http_client.c:121:Username is reset:PASS

-----------------------
5 Tests 0 Failures 0 Ignored 
OK

@dobairoland dobairoland requested review from mahavirj and Spritetm and removed request for Spritetm April 3, 2019 13:31
@dobairoland dobairoland self-assigned this Apr 3, 2019
@dobairoland
Copy link
Collaborator

@mahavirj @david-cermak PTAL

Copy link
Collaborator

@david-cermak david-cermak left a comment

Choose a reason for hiding this comment

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

Hi @hongquan

Thanks for sending this fix. I posted some comments to your code, mainly minor/cosmetic suggestions or open points to discuss. I would ask for an update form your side though, at least related to the unit test file.

Thanks for your contribution, nice and solid work, mainly appreciated the unit tests for the added fix.

components/esp_http_client/esp_http_client.c Outdated Show resolved Hide resolved
components/esp_http_client/include/esp_http_client.h Outdated Show resolved Hide resolved
components/esp_http_client/test/test_http_client.c Outdated Show resolved Hide resolved
components/esp_http_client/test/test_http_client.c Outdated Show resolved Hide resolved
components/esp_http_client/test/test_http_client.c Outdated Show resolved Hide resolved
@hongquan
Copy link
Contributor Author

hongquan commented Apr 8, 2019

Hi @david-cermak I've just updated the code.

New test result:

Press ENTER to see the list of tests.
Running most_common_use...
Note: tcpip_adapter_init() has been called. Until next reset, TCP/IP task will periodicially allocate memory and consume CPU time.
E (2454) HTTP_CLIENT: config should have either URL or host & path
/home/quan/Works/Forks/esp-idf/components/esp_http_client/test/test_http_client.c:27:most_common_use:PASS
Running get_username_password...
/home/quan/Works/Forks/esp-idf/components/esp_http_client/test/test_http_client.c:53:get_username_password:PASS
Running username_not_lost...
/home/quan/Works/Forks/esp-idf/components/esp_http_client/test/test_http_client.c:82:username_not_lost:PASS
Running username_is_reset...
D (2548) HTTP_CLIENT: New host assign = httpbin.org
/home/quan/Works/Forks/esp-idf/components/esp_http_client/test/test_http_client.c:109:username_is_reset:PASS

-----------------------
4 Tests 0 Failures 0 Ignored 
OK

Copy link
Collaborator

@david-cermak david-cermak left a comment

Choose a reason for hiding this comment

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

Thanks for such a quick update. Looks good to me, but please just squash the commits (or at least melt the two latter to the previous two). In perfect case possibly create just two commits, one fixing the issue, another one adding unit tests.

@hongquan
Copy link
Contributor Author

hongquan commented Apr 8, 2019

Hi @david-cermak

I've rebased the commits to two.

@dobairoland
Copy link
Collaborator

Thank you @hongquan for your contribution. I'm submitting this for internal review. I'll contact you if other requests emerge. Otherwise, this PR will be closed when it gets merged into master.

Thank you Mahavir and David for the review.

@igrr igrr closed this in 4a6c503 Apr 17, 2019
malouf-jason pushed a commit to malouf-jason/esp-idf that referenced this pull request Sep 5, 2019
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.

5 participants