feat: update gradle dependencies

[mobilekosmos]

• I am not disclosing a vulnerability.
• I am creating this PR in reference to an issue.

Related issue: #1159

• Add tests
• Add changes to documentation (guides, repository pages, in-code descriptions)
• A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

[parse-github-assistant]

Thanks for opening this pull request!

• ❌ Please edit your post and use the provided template when creating a new pull request. This helps everyone to understand your post better and asks for essential information to quicker review the pull request.

[parse-github-assistant]

I will reformat the title to use the proper commit message syntax.

[azlekov]

Great job @mobilekosmos

LGTM

[mtrezza]

I just noticed the CI is failing, do we need a separate PR to fix something or is this related to this PR?

[mobilekosmos]

It seems that all the mockito imports are not being resolved, could also see locally in my IDE, it happens when updating mockito to latest version, must investigate solution.
Reasson: https://github.com/mockito/mockito/releases/tag/v4.0.0

[mobilekosmos]

How to fix the spotless failing? Wouldn't it be possible to let the CI do the formatting automatically?

[mtrezza]

You should be able to run spotless to auto-correct what's possible locally and commit these changes.

[mobilekosmos]

"First-time contributors need a maintainer to approve running workflows."

[mtrezza]

@mobilekosmos Could you please take a look at the failing CI?

[codecov]

Codecov Report

Merging #1163 (b01922d) into master (ec7bd03) will decrease coverage by 0.52%.
The diff coverage is n/a.

❗ Current head b01922d differs from pull request most recent head 767da96. Consider uploading reports for the commit 767da96 to get more accurate results

@@             Coverage Diff              @@
##             master    #1163      +/-   ##
============================================
- Coverage     67.31%   66.79%   -0.53%     
+ Complexity     2285     2248      -37     
============================================
  Files           122      121       -1     
  Lines          9962     9892      -70     
  Branches       1343     1332      -11     
============================================
- Hits           6706     6607      -99     
- Misses         2735     2773      +38     
+ Partials        521      512       -9     

Impacted Files	Coverage Δ
parse/src/main/java/com/parse/ParsePlugins.java	31.25% <0.00%> (-18.75%)	⬇️
parse/src/main/java/com/parse/ParseFileUtils.java	36.70% <0.00%> (-9.08%)	⬇️
parse/src/main/java/com/parse/Parse.java	61.68% <0.00%> (-3.90%)	⬇️
parse/src/main/java/com/parse/ParseRequest.java	80.00% <0.00%> (-2.00%)	⬇️
parse/src/main/java/com/parse/ParseObject.java	61.21% <0.00%> (-0.14%)	⬇️
...in/java/com/parse/ParseCacheDirMigrationUtils.java
...rc/main/java/com/parse/ParseRESTObjectCommand.java	84.61% <0.00%> (+11.53%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c658995...767da96. Read the comment docs.

[mobilekosmos]

Despite such minor changes, the pain of a PR is big.

[mtrezza]

This is upgrading the Facebook SDK from 12 to 13. Does this imply a breaking change for developers? If so we'd need to make this a major version increase.

[mobilekosmos]

Does this imply a breaking change for developers?

Shouldn't automated tests respond to this?

[mobilekosmos]

Btw. it seems that Facebook's Android SDK 13 urges to "migrate" to it:
Changelog
Facebook SDK 13 blogpost

[mobilekosmos]

After seeing the amount of files changed with this PR I'm asking myself if it wouldn't be better, at least in the future, to accept only one dependency update per each PR, more burocratic and painfull again, but maybe a better approach? Not sure yet.

[mtrezza]

@mobilekosmos Yes, I would not mix a major version upgrade of the FB SDK with other dependency upgrades, because it's an essential part of the SDK with its own complexities. Depending on what the breaking change in the FB SDK 13 is, we may have to declare a breaking change for the Parse Android SDK if developers cannot simply upgrade the SDK without having to change their code. Could you take a look at the FB SDK changelog?

[azlekov]

@L3K0V Could we get your opinion on whether upgrading the FB SDK by a major version should be considered a breaking change and we need to increment the major version of the Parse Android SDK as well?

@mtrezza I tried to follow all breaking changes on the Facebook SDK and check if something is needed to be change as a breaking change on this SDK, but didn't see anything. Still, following the discussions and the issues it looks that there are some issues, but I'm no sure if it's a misconfiguration of the app or not. In #1164 from the screenshots I saw that the app is not configured.

About:

When someone adds the Parse Android SDK, the FB SDK is added as a dependency. Could they then just use another feature of that FB SDK dependency in their project, unrelated to the Parse SDK? If so, upgrading the Parse SDK may break their other FB related code.

Could they then just use another feature of that FB SDK dependency in their project, unrelated to the Parse SDK?

Yes, the Facebook SDK is api dependency and is available to the developer in his app.

If so, upgrading the Parse SDK may break their other FB related code.

Yes, it's possible

[mtrezza]

Thanks, so we have to do a major version increment when we merge this.

[mtrezza]

I'm suggesting to fix #1158 first before we merge this, because any release without the migration logic is for most people likely a useless release.

[mtrezza]

Good news, #1158 has been merged, so we can resume this PR.

The question is still, whether a major version increment by FB SDK means the Parse SDK has to do a major increment as well. Let's explore this:

The FB SDK is a transitive dependency (and compile/api are long deprecated in favor of implementation), so it's inaccessible for the developer's app. If a developer wants to directly call any FB SDK method, they'd have to separately add the FB SDK as dependency to their app.

For developers who do have the Facebook SDK and Parse SDK added to their project, this creates a challenge due to Gradle's dependency resolution management.

For example, if Parse Android SDK 3.0.1 is added to an app that also has added:

implementation 'com.facebook.android:facebook-android-sdk:13.2.0'

Gradle will resolve this by overriding the Parse Android SDK's dependency:

com.facebook.android:facebook-login:12.1.0 -> 13.2.0

This happens in the background and looks problematic, because Parse SDK is tested with 12.1.0, not 13.2.0. It seems that the issue is that we add the FB Login SDK in Parse SDK like this:

 api "com.facebook.android:facebook-login:12.1.0"

We are saying that the Parse SDK works with any FB SDK version >= 12.1.0 which isn't correct. It should trigger a resolution error because the app requires FB SDK >= 13.2.0 and the Parse SDK is only tested with 12.1.0, and we can assume FB follows semver and it will be compatible until <13.0.0.

So we would need to add a dependency constraint, which currently should be the range [13.2.0, 14.0[ (note that 13.2+or 13.+ would be incorrect). If this PR bumps the FB SDK 13.2.0, the dependency should be set to:

com.facebook.android:facebook-login:[13.2.0, 14.0[`

I don't think we need to increment by a major version, since this is a "dependency breaking change" that gradle will identify and report if there is a resolution conflict with the developer's app. That of course requires the developer to properly set the version ranges of their app's dependencies, which is the developer's responsibility. So we don't have to worry about it.

What do you think @L3K0V, @rommansabbir?

[azlekov]

compile / api are long deprecated in favor of implementation

That's not correct, only compile is deprecated, api is used for a transitive dependencies.

Facebook SDK dependency can be changed from api to implementation and be used only within the library. In this scenario the developer should take care of adding its own version of the Facebook SDK and use it accordingly to its source code. I'm not aware how the Facebook SDK is configured to work and what will happen if there are two versions - one used by the app and another used by the Parse SDK and if the dependency resolution will cause some troubles.

The point having the Facebook as a api dependency I think is to push the user to use the version bundled with the Parse SDK and do the necessary adjustments to their own Facebook code.

Another approach will be to make the Facebook SDK runtime or compileRuntime dependency and to expect the SDK to be there in the app, but in such case the Parse SDK should take care to cover as many as possible Facebook versions and their breaking changes inside the implementation.

I cannot take a decision here, not until I research how is done in other similar cases and situations.

[mman]

compile / api are long deprecated in favor of implementation

That's not correct, only compile is deprecated, api is used for a transitive dependencies.

Facebook SDK dependency can be changed from api to implementation and be used only within the library. In this scenario the developer should take care of adding its own version of the Facebook SDK and use it accordingly to its source code. I'm not aware how the Facebook SDK is configured to work and what will happen if there are two versions - one used by the app and another used by the Parse SDK and if the dependency resolution will cause some troubles.

AFAIK It will cause troubles if two different versions of Facebook SDK will end up being used. I have experienced this myself many times. The Android Studio compiler will eventually flag this as "duplicate class" error since Java can not use two .jar files providing the same classes in isolation.

This can work in C/Objective-C/Swift world where static method are isolated per .o file and multiple versions of the same method can this way be linked into the same executable, bit it does not work like this in java world as far as I know.

[mtrezza]

That's not correct, only compile is deprecated, api is used for a transitive dependencies.

Yes, sorry for the confusion, should have phrased that better.

I'm not aware how the Facebook SDK is configured to work and what will happen if there are two versions - one used by the app and another used by the Parse SDK and if the dependency resolution will cause some troubles.

AFAIK It will cause troubles if two different versions of Facebook SDK will end up being used. ... The Android Studio compiler will eventually flag this as "duplicate class" error.

There can't be 2 different FB SDK versions used at the same time. Gradle will try to decide on a single version. If it cannot, it will throw an error. If a developer implements their own FB SDK with a higher version, gradle will override the FB SDK version in the Parse SDK. We have to tell gradle that we did not test the Parse SDK with any version higher than 12.1.0. We can only assume that it will be compatible with >=12.1.0 <13.0.0 according to semver. If we don't specify an upper range and only say >=12.1.0, we are effectively misleading gradle (and the developer).

If a developer tries to implement a FB SDK that is incompatible with Parse SDK's dependency of the FB SDK, gradle should throw an error, that is nothing bad but necessary. Otherwise the Parse SDK will cause runtime errors when it tries to access an API that is not available in the FB SDK.

In addition, we are only implementing the FB Login SDK, that means chances are high that a developer adds their own FB SDK to the app, because all other FB modules are missing.

[azlekov]

@mman my point was about the resolution strategy of Gradle which version will pick of many and what will be the behaviour after that. In your experience and from your perspective what should be the approach here?

a) Facebook SDK as runtime dependency covering many cases in the Parse SDK
b) Facebook SDK as api dependency and the developer to use same version in their app + adjustments on their own FB code
c) Facebook SDK as implementation and hoping Gradle to pick up most suitable version between SDK and the app.

Maybe is a manner of how do we want to 'communicate' to developers, do we want to push them to use bundled version, or their own or something else.

[azlekov]

I haven't used before the versions constraints @mtrezza, where you can specify a range. It sounds promising but again maybe we should discuss whenever the Parse SDK bundle the Facebook SDK, bundle + export (api) or expect it runtime.

[mtrezza]

my point was about the resolution strategy of Gradle which version will pick of many and what will be the behaviour after that. In your experience and from your perspective what should be the approach here?

That's answered in the dependency resolution management and my previous 2 comments.

Maybe is a manner of how do we want to 'communicate' to developers, do we want to push them to use bundled version, or their own or something else

Gradle communicates that to the developer. Even if we specify an upper range, the developer can still decide to force override the FB SDK version in the Parse SDK to resolve the resolution conflict. That will then be a deliberate choice, with the developer being aware that is may cause runtime errors because it's not tested. But it should never happen in the background without the developer knowing. A changelog may be read or not read, but gradle will actually warn the developer in a way that is hard to overlook.

whenever the Parse SDK bundle the Facebook SDK, bundle + export (api) or expect it runtime.

Good question. I think we can separate that from the range discussion, whatever we choose, we need to define a range because we cannot future-test the Parse SDK with unreleased FB SDK versions. How about we add the range in this PR and open a new issue to discuss the larger question you posed? That will also give the discussion more visibility.

[azlekov]

How about we add the range in this PR and open a new issue to discuss the larger question you posed?

Sounds great 👍

[mtrezza]

So the conclusion would be:

• Set upper range for FB Login SDK to >=13.1.0 <14.0.0 in this PR

• Merge this PR with a major version increase, as it may be a breaking change. The FB Login SDK is currently added as api, so it can transitively accessed if the developer adds the Parse SDK also as api to their app. Since the FB SDK has some braking changes with 13.x, we need to consider that also a breaking change for the Parse SDK.

@mman What do you think?

[mtrezza]

Change to

api "com.facebook.android:facebook-login:[13.2.0, 14.0["

[mtrezza]

@mobilekosmos Would you take a look at the small change requested and the failing CI so we can merge this? I assume the failing test is caused by this PR, because we just merged #1158 with all tests passing.

[mtrezza]

Does anyone want to continue this PR to get it merged? It seems to need just a rebase and a trivial code change. It seems that @mobilekosmos has abandoned it.

@parse-community/android-sdk

[rommansabbir]

Does anyone want to continue this PR to get it merged? It seems to need just a rebase and a trivial code change. It seems that @mobilekosmos has abandoned it.

I will try to start again with this PR, once I'm back to my working place.

[mtrezza]

Cool, @rommansabbir can I add you to the Parse Android SDK review team? No obligations, but you would get a notification as member of @parse-community/android-sdk-review once a PR is ready for review and can do a review to get it merge-ready.

[rommansabbir]

Cool, @rommansabbir can I add you to the Parse Android SDK review team? No obligations, but you would get a notification as member of @parse-community/android-sdk-review once a PR is ready for review and can do a review to get it merge-ready.

Yes, that would be amazing!

[rommansabbir]

@mtrezza I have updated FB SDK (api) version to a specific range and run all the tests from the existing tests, all passed but I'm unable to push my changes to @mobilekosmos HEAD:master to that the changes could refelect in this PR.

@mobilekosmos can you check, if it's allowed to push commits to this PR?

Or, else I just create a new PR and refer to this one? @mman @mtrezza

[mtrezza]

New PR please, I will then go ahead and close this PR.

[mtrezza]

Closing, replaced by #1172.

Thanks for the initial work, @mobilekosmos!