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

add custom PatchMethod class to use http PATCH requests with the Nextcloud SSO api #8818

Merged
merged 4 commits into from
Sep 27, 2021

Conversation

binsky08
Copy link
Contributor

@binsky08 binsky08 commented Aug 6, 2021

should solve nextcloud/Android-SingleSignOn#355

Seems like the used org.apache.commons.httpclient is end of life: https://hc.apache.org/httpclient-legacy/
Therefore I created a PatchMethod class based on the official PostMethod class. It works. :)

Testing

Writing tests is very important. Please try to write some tests for your PR.
If you need help, please do not hesitate to ask in this PR for help.

unit tests
instrumented tests
UI tests

  • Tests written, or not not needed

…cloud SSO api

Signed-off-by: binsky <timo@binsky.org>
@@ -0,0 +1,117 @@
package com.nextcloud.android.sso;
Copy link
Member

Choose a reason for hiding this comment

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

Please add license header.
If this is a copy from PostMethod please also note this somewhere.

Copy link
Contributor Author

@binsky08 binsky08 Aug 6, 2021

Choose a reason for hiding this comment

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

I haven't had to do a lot with licenses yet. @tobiasKaminsky Do you think that's okay?
Do I need that author and copyright part?

/*
 * Nextcloud SingleSignOn
 *
 * @author Timo Triebensky
 * Copyright (C) 2021 Timo Triebensky
 *
 * This program is free software: you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation, either version 3 of the License, or
 * (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program.  If not, see <http://www.gnu.org/licenses/>.
 *
 * More information here: https://github.com/abeluck/android-streams-ipc
 *
 * ====================================================================
 *
 * The required methods of this class are copied and customized from PostMethod.
 */

import java.util.Vector;

public class PatchMethod extends PostMethod {
// -------------------------------------------------------------- Constants
Copy link
Member

Choose a reason for hiding this comment

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

As we do not use such kind of comments, please remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed it

/**
* Log object for this class.
*/
private static final Log LOG = LogFactory.getLog(PatchMethod.class);
Copy link
Member

Choose a reason for hiding this comment

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

We use another log approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, Log_OC. I removed the log method that i took over from the parent class

* @since 2.0beta1
*/
protected boolean hasRequestContent() {
LOG.trace("enter PatchMethod.hasRequestContent()");
Copy link
Member

Choose a reason for hiding this comment

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

Please remove those logs, as we do not need them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed it

* <p>This method must be overwritten by sub-classes that implement
* alternative request content input methods</p>
*
* @since 2.0beta1
Copy link
Member

Choose a reason for hiding this comment

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

This is from c&p I assume, so please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed it

Copy link
Member

@tobiasKaminsky tobiasKaminsky left a comment

Choose a reason for hiding this comment

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

Adopt to our code syntax

Signed-off-by: binsky <timo@binsky.org>
@tobiasKaminsky
Copy link
Member

Cool, thanks for your code changes.
One last thing: as spotbugs increased, but it complains "only" about complexity, can you adjust number in findbugs-results.txt to 464?

…SSO api

Signed-off-by: binsky <timo@binsky.org>
@binsky08
Copy link
Contributor Author

binsky08 commented Aug 30, 2021

@tobiasKaminsky do i have to do anything else?

@nextcloud-android-bot
Copy link
Collaborator

@nextcloud-android-bot
Copy link
Collaborator

Codacy

Lint

TypemasterPR
Warnings6868
Errors00

SpotBugs (new)

Warning Type Number
Bad practice Warnings 28
Correctness Warnings 50
Internationalization Warnings 9
Malicious code vulnerability Warnings 156
Multithreaded correctness Warnings 9
Performance Warnings 72
Security Warnings 40
Dodgy code Warnings 98
Total 462

SpotBugs (master)

Warning Type Number
Bad practice Warnings 28
Correctness Warnings 50
Internationalization Warnings 9
Malicious code vulnerability Warnings 156
Multithreaded correctness Warnings 9
Performance Warnings 72
Security Warnings 40
Dodgy code Warnings 96
Total 460

@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.18.0 milestone Sep 27, 2021
@AndyScherzinger AndyScherzinger merged commit cfae25c into nextcloud:master Sep 27, 2021
@AndyScherzinger
Copy link
Member

AndyScherzinger commented Sep 27, 2021

@tobiasKaminsky do i have to do anything else?

Nope, merged and thank you very much for this @binsky08 💙

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants