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

Override fetch for Altas App Services #6165

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,28 @@
* `Realm.User.providerType` is deprecated, and will be remove in next major version. Use `Realm.User.identities` instead.

### Enhancements
* None
* Expose `Realm.App.Configuration.fetchOverride` to enable implementing a custom [`fetch`](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch) function. ([#6159](https://github.com/realm/realm-js/issues/6159))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should say AppConfiguration.fetchOverride rather than Realm.App.Configuration.fetchOverride due to how users currently can import it. Atm it seems like they can only import it via { AppConfiguration }.

Suggested change
* Expose `Realm.App.Configuration.fetchOverride` to enable implementing a custom [`fetch`](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch) function. ([#6159](https://github.com/realm/realm-js/issues/6159))
* Exposed `Realm.App.Configuration.fetchOverride` to enable implementing a custom [`fetch`](https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch) function. ([#6159](https://github.com/realm/realm-js/issues/6159))


```js
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
```js
```typescript

const HTTP_METHOD: Record<number, string> = {
[0]: "GET",
[1]: "POST",
[2]: "PUT",
[3]: "PATCH",
[4]: "DELETE",
};

const customFetch = async (request: Request) => {
const { url, body, ...rest } = request;
if (typeof request.method == "number") {
rest.method = HTTP_METHOD[request.method];
}
const response = await fetch(url, rest); // `fetch` is assumed to be compliant with the Fetch API specification
return response;
};
Comment on lines +10 to +25
Copy link
Contributor

@elle-j elle-j Oct 16, 2023

Choose a reason for hiding this comment

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

I added a PR comment to integration-tests/tests/src/node/custom-fetch.ts explaining why I believe most of this can be simplified.

But also, for this CHANGELOG entry I think it could be good to show only what's necessary since users might have very different implementations of their custom fetch.

Suggested change
const HTTP_METHOD: Record<number, string> = {
[0]: "GET",
[1]: "POST",
[2]: "PUT",
[3]: "PATCH",
[4]: "DELETE",
};
const customFetch = async (request: Request) => {
const { url, body, ...rest } = request;
if (typeof request.method == "number") {
rest.method = HTTP_METHOD[request.method];
}
const response = await fetch(url, rest); // `fetch` is assumed to be compliant with the Fetch API specification
return response;
};
const customFetch = (request: Request): Promise<Response> => {
// `fetch` is assumed to be compliant with the Fetch API specification
return fetch(request);
};


let app = new Realm.App({ id: "my-app-id", fetchOverride: customFetch });
```

### Fixed
* Outside migration functions, it is not possible to change the value of a primary key. ([#6161](https://github.com/realm/realm-js/issues/6161), since v12.0.0)
Expand Down
1 change: 1 addition & 0 deletions integration-tests/tests/src/hooks/import-app-before.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export type AppConfigurationRelaxed = {
timeout?: number;
multiplexSessions?: boolean;
baseFilePath?: string;
fetchOverride?: any;
Copy link
Member

Choose a reason for hiding this comment

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

My immediate reaction is that the "override" part of this name is redundant as all of these options override default values.

Suggested change
fetchOverride?: any;
fetch?: any;

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, would also be good to match the TS type here to whichever type is used on the SDK-exported type in App.ts.

};

export function importAppBefore(config: AppConfig | { config: AppConfig }, sdkConfig?: AppConfigurationRelaxed): void {
Expand Down
66 changes: 66 additions & 0 deletions integration-tests/tests/src/node/custom-fetch.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
////////////////////////////////////////////////////////////////////////////
//
// Copyright 2022 Realm Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//
////////////////////////////////////////////////////////////////////////////

import { expect } from "chai";
import { importAppBefore } from "../hooks";
import { buildAppConfig } from "../utils/build-app-config";
import fetch, { Request } from "node-fetch";
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding Response as part of my refactor suggestion.

Suggested change
import fetch, { Request } from "node-fetch";
import fetch, { Request, Response } from "node-fetch";

import Realm from "realm";

describe("custom fetch", () => {
let called = false;

const HTTP_METHOD: Record<number, string> = {
[0]: "GET",
[1]: "POST",
[2]: "PUT",
[3]: "PATCH",
[4]: "DELETE",
};
Comment on lines +28 to +34
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it'll make more sense to export this somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe, but I have a feeling that we might leak too many implementation details.


const customFetch = async (request: Request) => {
called = true;
const { url, body, ...rest } = request;
if (typeof request.method == "number") {
rest.method = HTTP_METHOD[request.method];
}
const response = await fetch(url, rest);
return response;
};
Comment on lines +36 to +44
Copy link
Member

Choose a reason for hiding this comment

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

An alternative could be to implement this as a wrapping of the default fetch from @realm/network-transport.

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 am split here: part of me wants to wrap it as much as possible, but a part of me wishes to give the user full flexibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this solution is good, otherwise we would also have to provide a mechanism to override the default AbortController as well. This basically lets the user override @realm/network-transport.
I guess the crux of the issue, is the user that reported the original issue is using electron and this is using the node bindings instead of the browser bindings to fetch.

Comment on lines +28 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually don't think you need the destructuring nor the typeof check in this case.

Since you're using node-fetch, the request.method will always be a string. Additionally, the destructuring can be removed because the 1st arg to fetch can be the Request itself.

The reason we were doing the typeof dance in our network.ts implementation is because the request could in our case also be a binding.Request (on which the method is an integer).

Suggested change
const HTTP_METHOD: Record<number, string> = {
[0]: "GET",
[1]: "POST",
[2]: "PUT",
[3]: "PATCH",
[4]: "DELETE",
};
const customFetch = async (request: Request) => {
called = true;
const { url, body, ...rest } = request;
if (typeof request.method == "number") {
rest.method = HTTP_METHOD[request.method];
}
const response = await fetch(url, rest);
return response;
};
const customFetch = (request: Request): Promise<Response> => {
called = true;
return fetch(request);
};


importAppBefore(buildAppConfig("with-anon").anonAuth(), { fetchOverride: customFetch });
afterEach(() => {
Realm.clearTestState();
});

it("custom fetch is called", async function (this: Mocha.Context & AppContext & RealmContext) {
let user;
called = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since called is shared among all tests in this suite (if more tests were to be added), perhaps we could also set it to false in a before/afterEach to not be too dependent on the need to know that it should always be set to false first?

try {
expect(this.app).instanceOf(Realm.App);
const credentials = Realm.Credentials.anonymous();
user = await this.app.logIn(credentials);
expect(called).equals(true);
expect(user).instanceOf(Realm.User);
expect(user.deviceId).to.not.be.null;
expect(user.providerType).equals("anon-user");
} finally {
await user?.logOut();
}
});
});
1 change: 1 addition & 0 deletions integration-tests/tests/src/node/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,6 @@

import "./analytics";
import "./clean-exit";
import "./custom-fetch";
import "./path";
import "./sync-proxy";
4 changes: 3 additions & 1 deletion integration-tests/tests/src/tests/sync/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ import Realm, { AppConfiguration, BSON, MetadataMode } from "realm";

import { importAppBefore } from "../../hooks";
import { generatePartition } from "../../utils/generators";
import { baseUrl } from "../../utils/import-app";
import { baseUrl, Response } from "../../utils/import-app";
import { select } from "../../utils/select";
import { buildAppConfig } from "../../utils/build-app-config";
import { Fetch, Request } from "@realm/network-transport";
import { fetch } from "../../utils/fetch";
Comment on lines +24 to +28
Copy link
Contributor

@elle-j elle-j Oct 16, 2023

Choose a reason for hiding this comment

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

I think the added imports (Response, Fetch, Request, fetch) can safely be removed. They don't seem to be used anywhere here.


const TestObjectSchema: Realm.ObjectSchema = {
primaryKey: "_id",
Expand Down
2 changes: 1 addition & 1 deletion packages/realm/realm-constants.json
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"REALM_ANONYMIZED_BUNDLE_ID":"tkgif/+3l1e9wStGJp2TOngAK3UcQ2u7OM8ZYJU5JYo="}
{"REALM_ANONYMIZED_BUNDLE_ID":"1RmJBlqbKuzyRiPm4AsdIIxe8xlRUntGcGFEwUnUh6A="}
15 changes: 14 additions & 1 deletion packages/realm/src/app-services/App.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
//
////////////////////////////////////////////////////////////////////////////

import { NetworkType, inject } from "src/platform/network";
import {
AnyUser,
Credentials,
Expand Down Expand Up @@ -140,6 +141,13 @@ export type AppConfiguration = {
* @since 12.2.0
*/
metadata?: Metadata;

/**
* Specify a custom `fetch` implementation.
* @since 12.3.0
* @experimental This API is experimental and may change or be removed.
*/
fetchOverride?: unknown;
Copy link
Contributor

@takameyer takameyer Oct 16, 2023

Choose a reason for hiding this comment

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

Should we specify this type to match the NetworkType from network.ts

  fetch(request: Request | binding.Request ): Promise<FetchResponse>;

Copy link
Contributor

@elle-j elle-j Oct 16, 2023

Choose a reason for hiding this comment

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

I think it should be similar to a specification, or if that is too specific for a custom fetch, perhaps just a function type such as () => Promise<unknown>. Also, I'd say we should try to not expose bindgen details to the public API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will there be unintended side effects if the fetch function doesn't match our specifications?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you referring to our NetworkType? Since our TS expects the fetch used (whether it'd be ours or a user-provided one) to be any of the functions defined in the NetworkType, the preferable option (from the way it's set up atm) seems to be fetch(request: Request): Promise<FetchResponse>;. In that case we'd need to export Request and FetchResponse.

But, safest would probably be to ask the user of their intended use (arg and return type) and if fetch(request: Request): Promise<FetchResponse>; would be sufficient for them.

Copy link
Contributor

@takameyer takameyer Oct 17, 2023

Choose a reason for hiding this comment

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

The user in this case just wants to replace node-fetch with window.fetch, since they are running realm in a browser environment in electron.

My main concern is this line. Here there is a transformation of the request happening depending on its contents. If a user is providing a custom fetch, will not doing this transformation cause an issue?

From this observation, perhaps @kraenhansen is correct in that we should directly override the fetch method within @realm/network-transport (a level deeper), and leave this one alone. Then we would also need to provide mechanisms to override the AbortController.

It's probably worth while doing a manual test in React Native, to see if such an override could cause significant issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it! 😎 I'll add it to our Round Table and let's discuss more :)

export function overrideFetch(fetch: (request: Request) => Promise<FetchResponse>) {
  baseFetch = fetch;
}

Copy link
Member

Choose a reason for hiding this comment

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

My immediate feeling is that I like the declarative API of keeping this a configuration of the App. Possibly providing a way to override the default values for those config values, but then again - how many developers do we expect to have more than one App instance per application?

I like ☝️ over this more procedural overrideFetch function suggestion as that minimize the state in the library, which also has the added benefit of allowing two App instances to fetch using different implementations concurrently.

Copy link
Contributor

Choose a reason for hiding this comment

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

The intention of the overrideFetch suggestion keeps the configuration through App, not for the developer to call this directly. The point being, to keep the internal logic of network:fetch (calls to toFetchRequest), and give the opportunity to override the fetch within the network:fetch method.
I was not proposing to remove the configuration by App, but to clear up that we would be overriding the internal network:fetch logic with the current PR state.

Copy link
Member

@kraenhansen kraenhansen Oct 23, 2023

Choose a reason for hiding this comment

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

@takameyer I don't see how that'll work. The App (or something with access to the fetch passed via the App's configuration) would have to call overrideFetch, passing in the fetch before every single request to the server, as it'll be a shared state between two App instances, that would have to be constantly switched between the two.

Copy link
Contributor

@takameyer takameyer Oct 23, 2023

Choose a reason for hiding this comment

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

@kraenhansen Then we should write this in a way that works in the case you are suggesting. The code I wrote above was only an example, but the intention should be clear.

In any case, I think we are misunderstanding each other and would enjoy discussing this further when you return.

};

/**
Expand Down Expand Up @@ -261,7 +269,8 @@ export class App<
constructor(configOrId: AppConfiguration | string) {
const config: AppConfiguration = typeof configOrId === "string" ? { id: configOrId } : configOrId;
assert.object(config, "config");
const { id, baseUrl, app, timeout, multiplexSessions = true, baseFilePath, metadata } = config;
const { id, baseUrl, app, timeout, multiplexSessions = true, baseFilePath, metadata, fetchOverride } = config;

assert.string(id, "id");
if (timeout !== undefined) {
assert.number(timeout, "timeout");
Expand All @@ -277,6 +286,10 @@ export class App<
}
}

if (fetchOverride) {
inject({ fetch: fetchOverride } as NetworkType);
}
Comment on lines 288 to +291
Copy link
Contributor

@elle-j elle-j Oct 16, 2023

Choose a reason for hiding this comment

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

If we decide to add a more explicit type than any or unknown to the fetchOverride in the app config (I think at least specifying that it should be a function could be good), then what do you think of adding an explicit check for undefined here? This way, we can let users know if they have provided an invalid falsy value in addition to an invalid truthy value by asserting that it's a function.

Suggested change
if (fetchOverride) {
inject({ fetch: fetchOverride } as NetworkType);
}
if (fetchOverride !== undefined) {
assert.function(fetchOverride, "fetchOverride");
inject({ fetch: fetchOverride } as NetworkType);
}


fs.ensureDirectoryForFile(fs.joinPaths(baseFilePath || fs.getDefaultDirectoryPath(), "mongodb-realm"));
// TODO: This used getSharedApp in the legacy SDK, but it's failing AppTests
this.internal = binding.App.getUncachedApp(
Expand Down
4 changes: 2 additions & 2 deletions packages/realm/src/platform/network.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export type { FetchHeaders, Request };
const debug = extendDebug("network");
const transport = new DefaultNetworkTransport();

type NetworkType = {
export type NetworkType = {
fetch(request: Request): Promise<FetchResponse>;
fetch(request: binding.Request): Promise<FetchResponse>;
};
Expand Down Expand Up @@ -60,5 +60,5 @@ export const network: NetworkType = {
};

export function inject(injected: NetworkType) {
Object.freeze(Object.assign(network, injected));
Object.assign(network, injected);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did using .freeze() cause an issue here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless this inject called multiple times, the freeze shouldn't be an issue.

Copy link
Member

@kraenhansen kraenhansen Oct 18, 2023

Choose a reason for hiding this comment

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

Yeah - my suggestion (to @kneth) to remove the freeze was only if we choose to expose the inject function to end-users as a means of overriding the fetch implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha 🙂

}
Loading