taskadapter / redmine-java-api

Redmine Java API
Apache License 2.0
269 stars 162 forks source link

Return null to caller for 204 http no response case #355

Closed haiyanghaiyang closed 3 years ago

haiyanghaiyang commented 4 years ago

Curent HttpResponse handler assumes HttpResponse entity is not null. But for 204 no response case, it raises exception. I got the problem with Gerrit integration with Redmine 4.1.1, where Gerrit its-redmine plugin submits three times when exception happens. When this change, there's no exception raised by redmine-java-api.

alexeyOnGitHub commented 4 years ago

let's have integration tests verifying this change. they should pass against the regular (non-modded) Redmine instance (a recent version).

haiyanghaiyang commented 4 years ago

I setup Redmine 4.1.1 without extra plugin, with information configured in src/test/resources/api_test.properties. The test can login system but some authentication failure. How can I get it passed? I don't think it is caused by the PR. Have you verified last release against 4.1.1?

com.taskadapter.redmineapi.ProjectIntegrationIT > getNewsDoesNotFailForTempProject FAILED com.taskadapter.redmineapi.NotAuthorizedException at ProjectIntegrationIT.java:536

com.taskadapter.redmineapi.CustomFieldDefinitionsIT > testGetCustomFields FAILED java.lang.AssertionError at CustomFieldDefinitionsIT.java:38

com.taskadapter.redmineapi.TimeEntryManagerIT > testTimeEntryComments FAILED com.taskadapter.redmineapi.RedmineProcessingException at TimeEntryManagerIT.java:115

com.taskadapter.redmineapi.TimeEntryManagerIT > testTimeEntryDefaults FAILED com.taskadapter.redmineapi.RedmineProcessingException at TimeEntryManagerIT.java:76

com.taskadapter.redmineapi.TimeEntryManagerIT > testCreateGetTimeEntry FAILED com.taskadapter.redmineapi.RedmineProcessingException at TimeEntryManagerIT.java:148

com.taskadapter.redmineapi.TimeEntryManagerIT > testViolateTimeEntryConstraint_ProjectOrIssueID FAILED com.taskadapter.redmineapi.NotAuthorizedException at TimeEntryManagerIT.java:233

com.taskadapter.redmineapi.TimeEntryManagerIT > testCreateDeleteTimeEntry FAILED java.lang.Exception Caused by: com.taskadapter.redmineapi.RedmineProcessingException at TimeEntryManagerIT.java:171

com.taskadapter.redmineapi.TimeEntryManagerIT > timeEntriesAreFoundByFreeFormSearch FAILED com.taskadapter.redmineapi.RedmineProcessingException at TimeEntryManagerIT.java:209

com.taskadapter.redmineapi.TimeEntryManagerIT > testGetTimeEntriesForIssue FAILED com.taskadapter.redmineapi.RedmineProcessingException at TimeEntryManagerIT.java:209

com.taskadapter.redmineapi.IssueManagerIT > classMethod FAILED java.lang.RuntimeException at IssueManagerIT.java:77 Caused by: com.taskadapter.redmineapi.RedmineAuthenticationException at IssueManagerIT.java:77

com.taskadapter.redmineapi.IssueManagerIT > classMethod FAILED java.lang.NullPointerException at IssueManagerIT.java:103

150 tests completed, 11 failed, 8 skipped

alexeyOnGitHub commented 4 years ago

note - I am no longer actively working on this project. I can do a code review and merge the changes when people provide a verified PR with passing tests, but that's about it. please check the docs in this repo. you may need to update the redmine config to provide proper credentials for your server/install, also check a few test setup steps - there are a few tests require some manual one-time setup (because Redmine API does not support remote operations required for that setup). those (very few) tests will fail and you can see comments inside those tests.

haiyanghaiyang commented 4 years ago

This error are there before the PR. Without the PR, the test has much more error with Redmine 4.1.1. I think it needs extra effort and other PR to fix other error.

alexeyOnGitHub commented 3 years ago

I looked at the communicator implementation. empty response may or may not be valid for the provided handler. this change would short-circuit the whole handler processing chain and decide that an empty response must be returned as null, instead of letting the provided handler do its job. this change is not ready to be merged.

alexeyOnGitHub commented 3 years ago

this issue if fixed in https://github.com/taskadapter/redmine-java-api/pull/362