mobilekosmos commented 2 years ago

[x] I am not disclosing a vulnerability.
[x] 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)
[x] A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

parse-github-assistant[bot] commented 2 years ago

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[bot] commented 2 years ago

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

mtrezza commented 2 years ago

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

mobilekosmos commented 2 years ago

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 commented 2 years ago

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

mtrezza commented 2 years ago

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

mobilekosmos commented 2 years ago

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

mtrezza commented 2 years ago

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

codecov[bot] commented 2 years ago

Codecov Report

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

:exclamation: 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%)`	:arrow_down:
parse/src/main/java/com/parse/ParseFileUtils.java	`36.70% <0.00%> (-9.08%)`	:arrow_down:
parse/src/main/java/com/parse/Parse.java	`61.68% <0.00%> (-3.90%)`	:arrow_down:
parse/src/main/java/com/parse/ParseRequest.java	`80.00% <0.00%> (-2.00%)`	:arrow_down:
parse/src/main/java/com/parse/ParseObject.java	`61.21% <0.00%> (-0.14%)`	:arrow_down:
...in/java/com/parse/ParseCacheDirMigrationUtils.java
...rc/main/java/com/parse/ParseRESTObjectCommand.java	`84.61% <0.00%> (+11.53%)`	:arrow_up:

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 commented 2 years ago

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

mobilekosmos commented 2 years ago

Does this imply a breaking change for developers?

Shouldn't automated tests respond to this?

mobilekosmos commented 2 years ago

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

mobilekosmos commented 2 years ago

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 commented 2 years ago

@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?

mobilekosmos commented 2 years ago

The thing is if I understood it correctly apps using an old version of the facebook sdk will stop working anyways

mtrezza commented 2 years ago

As I understand it, only FB Login will not work, but could an app use other functionality of the FB SDK when it's added as a dependency of the Parse Android SDK? (Reminds me that we probably shouldn't hard code the FB SDK into the Parse SDK.)

mobilekosmos commented 2 years ago

It would be a topic for another discussion, but the library should be modularized anyways, the main gradle Parse-SDK shouldn't contain ALL the modules it currently has, if you need facebook, twitter, etc. you must add parse-twitter separately and so on, google play services for example works like that. Regarding this PR, do what you want guys, I just wanted to help, but if it's so difficult I cannot invest more time.

mtrezza commented 2 years ago

We need to know whether this needs to be merged as a breaking change, that's all as far as I understand.

mtrezza commented 2 years ago

@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?

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.

azlekov commented 2 years ago

@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 commented 2 years ago

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

mtrezza commented 2 years ago

I'm suggesting to fix https://github.com/parse-community/Parse-SDK-Android/issues/1158 first before we merge this, because any release without the migration logic is for most people likely a useless release.

mtrezza commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

Sounds great 👍

mtrezza commented 2 years ago

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 commented 2 years ago

@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 https://github.com/parse-community/Parse-SDK-Android/issues/1158 with all tests passing.

mtrezza commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

mtrezza commented 2 years ago

Closing, replaced by https://github.com/parse-community/Parse-SDK-Android/pull/1172.

Thanks for the initial work, @mobilekosmos!

parse-community / Parse-SDK-Android

feat: update gradle dependencies #1163

Thanks for opening this pull request!

Codecov Report