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

Functions support #1110

Merged
merged 74 commits into from
Jan 16, 2023
Merged

Functions support #1110

merged 74 commits into from
Jan 16, 2023

Conversation

clementetb
Copy link
Contributor

@clementetb clementetb commented Nov 4, 2022

App function support

Add support for authenticated users to invoke App Services Application functions via the SDK.

Due to the serialization engine does not support third-party libraries yet, there are some limitations in what types can be used as arguments and return types:

  • Primitives, Bson, lists, and maps are valid argument types.
  • Results can only be deserialized to primitives or Bson types.

These limitations would be lifted once the Kserializer decoder for third-party libraries becomes stable.

The current implementation of the Ejson decoder does a case-by-case decoding based on the deserializationStrategy used. This would allow us to avoid future breaking changes, as it uses the same function signatures that we would be using in the future.

Pending

  • Use latest kserializer version.
  • Add tests for RealmInstant new extension functions.
  • Use kbson stable version.
  • Update the Changelog with the Kbson library version.

Depends on

@clementetb clementetb self-assigned this Nov 4, 2022
@cla-bot cla-bot bot added the cla: yes label Nov 4, 2022
@clementetb clementetb marked this pull request as ready for review November 16, 2022 11:18
Copy link
Contributor

@rorbech rorbech left a comment

Choose a reason for hiding this comment

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

Looks way nicer now 🎉 ... I am just not landed on whether it makes sense to have the public API with both T and the KType 🤔

Further, there is also missing some tests when arguments and return value cannot be encoded and decoded with the default BsonEncoder.

*
* @throws FunctionExecutionException if the function failed in some way.
*/
public suspend fun <T> call(
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit annoying that we cannot somehow tie the T with resultType ... Since there is no way to get the KType from a class at runtime, then there is no use cases for this method besides being called from the inlined one. Should we rather internalize this method then 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Also a bit funny to have the generic parameter throughout the BsonConverter.decodeFromBsonValue but maybe it is ok 🤔 ... But if we keep the T along with a KType that is not bound to be the same then we should definitely be very explicit about that it will throw a ClassCastException if they don't match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I didn't think it thoughtfully. Yes, we might like to internalize the method. And yes, the T value doesn't make sense in the BsonEncoder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made it internal and accessible to the extension function via de @PublishedApi annotation.


@Test
fun decodeFromBsonElement_throwsWrongType() {
(wrongPrimitiveAsserters + wrongRealmValueAsserters).forEach {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing serious, but since we don't really uses these variables elsewhere we could maybe just define them locally in the method. Also a bit hard to track because you need to look up the DecoderAsserter class to understand it. Why not just use a Triple and have all the logic here and then destruct them into named variables to that it was easier to follow. Anyway, nothing critical 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The asserters data structure has 4 parameters and is being reused in several tests.

Yes, we could use a Triple because not all parameters are used in every test, but it would mean that we would have to define multiple similar datasets in several tests..

clementetb and others added 7 commits January 9, 2023 10:14
…ternal/RealmInstantImpl.kt

Co-authored-by: Claus Rørbech <claus.rorbech@mongodb.com>
…ngodb/internal/BsonEncoder.kt

Co-authored-by: Claus Rørbech <claus.rorbech@mongodb.com>
…ngodb/Functions.kt

Co-authored-by: Christian Melchior <christian@ilios.dk>
… ct/app-functions

# Conflicts:
#	packages/library-sync/src/commonMain/kotlin/io/realm/kotlin/mongodb/internal/BsonEncoder.kt
clementetb and others added 3 commits January 12, 2023 14:00
…ngodb/internal/BsonEncoder.kt

Co-authored-by: Christian Melchior <christian@ilios.dk>
# Conflicts:
#	CHANGELOG.md
Copy link
Contributor

@rorbech rorbech left a comment

Choose a reason for hiding this comment

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

Awesome. Only some minor comments ... and then of course CI 😬 Seems to fail due to asserting on class names that are obfuscated. Either remove the actual class name in the expected methods or maybe add keep the specific classes for testing purposes, just as the other rules in https://github.com/realm/realm-kotlin/blob/main/packages/test-base/proguard-rules-test.pro

clementetb and others added 3 commits January 16, 2023 11:42
…ngodb/internal/BsonEncoder.kt

Co-authored-by: Claus Rørbech <claus.rorbech@mongodb.com>
@clementetb clementetb merged commit d4ebb86 into main Jan 16, 2023
@clementetb clementetb deleted the ct/app-functions branch January 16, 2023 13:48
@rorbech rorbech restored the ct/app-functions branch January 18, 2023 12:31
@rorbech rorbech deleted the ct/app-functions branch January 18, 2023 12:42
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants