kumar303 / hawkrest

Hawk HTTP Authorization for Django Rest Framework
BSD 3-Clause "New" or "Revised" License
19 stars 8 forks source link

FIX REST DELETE action. #47

Closed ghost closed 1 year ago

ghost commented 4 years ago

REST DELETE actions doe not provide content. So there is no 'Content-Type' header available. So use None. Fix #46 (https://github.com/kumar303/hawkrest/issues/46)

kumar303 commented 4 years ago

@JoshuaR1337 ah, right, this looks like the right thing to do. Can you add tests for it? https://github.com/kumar303/hawkrest/tree/620610733965941512c82f50852e0079df3bdb43/tests

ghost commented 3 years ago

Hi, I am sorry, but I do not know how to add a test command for this? There should be already a test for this? Else there was no test at all. And there has not been made any tests for 3 years now...

So when I changed the code as proposed here, and rerun the testing as is, nothing changed. Therefore, I say that this will not break existing functionality.

And as far I can judge, there are no tests for POST, PUT, and DELETE actions....

kumar303 commented 3 years ago

I think you should be able to copy this test and change the setup to self.request(method='DELETE', content_type=None), maybe?

So when I changed the code as proposed here, and rerun the testing as is, nothing changed. Therefore, I say that this will not break existing functionality.

I'm not too worried about it breaking existing functionality but I don't want to break this new functionality in the future on accident. Thus, it needs some test coverage. In other words, removing the line you added should break a test in the suite.

kumar303 commented 3 years ago

Also, here are instructions for how to run the test suite: https://hawkrest.readthedocs.io/en/latest/developers.html

Thanks for your interest in fixing this issue!

ghost commented 3 years ago

I am sorry to say, but I think this is not my job. As the tests are more then 3 year old, and they are all failing for python3, you cannot expect from me to fix all this.

The page https://hawkrest.readthedocs.io/en/latest/developers.html is NOT describing how to make tests. Running tox will produce a tons of errors, due to old testing code for invalid python versions.

So, I think you cannot ask me to add tests for a system that I do not know. I have no knowledge of TOX.

Your suggestion does not do anything. Because I will not get the error when making a DELETE request with the existing code. So I would expect errors when running the tests, which is not the case. Therefore I state this test scripts are not correct. And you cannot ask me to fix that!

Added to: test_middleware.py

def test_respond_ok_delete(self):
        req, sender = self.request(method='DELETE', content_type='application/json')

        response = HttpResponse('the response')
        res = self.mw.process_response(req, response)
        self.accept_response(res, sender)

and to test_authentication.py:

def test_hawk_delete(self):
        post_data = ''
        content_type = 'application/json'
        method = 'DELETE'
        sender = self._sender(content=post_data,
                              content_type=content_type,
                              method=method)
        req = self._request(sender,
                            content_type=content_type,
                            data=post_data,
                            method=method)

        assert isinstance(self.auth.authenticate(req)[0],
                          HawkAuthenticatedUser), (
            'Expected a successful authentication returning a user')

For both, using a content type of None, will break the testing itself, as the testing code does not check this, and always expect a string for content-type.

Again, I state the current testing code is not correct, outdated, and broken you can not expect from me to fix this all. Please pull this request... I have spent way more time about this discussion and stupid tests than it took the fix the code....

kumar303 commented 3 years ago

I understand that you had trouble getting the tests to run and that it was frustrating. Yes, sometimes it takes longer to write regression tests but this is the only way to make sure things work in the future.

This command will run the tests only on Python 2.7 (if you have it installed). Do these tests pass for you?

tox -e py27-django1.8-drf3.4

(I think the docs do need updating for this command, sorry.)

If you don't have Python 2.7, these other commands are currently defined in tox.ini:

tox -e py34-django1.8-drf3.5
tox -e py35-django1.8-drf3.5

If you can get one of them to run then maybe it would help you create new tests.

And, of course, I wouldn't expect you to fix anything that's currently broken. Thanks for spending time on fixing this bug.