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 endpoint does not work for Batch requests #385

Closed
sahejsingh opened this issue Jun 17, 2020 · 4 comments
Closed

Override endpoint does not work for Batch requests #385

sahejsingh opened this issue Jun 17, 2020 · 4 comments

Comments

@sahejsingh
Copy link

sahejsingh commented Jun 17, 2020

Describe your environment

  • Firebase Admin Go SDK v4.0.0

Step 3: Describe the problem

Same setup as #379 and #189, But this time try to send messages using batch api as mentioned here: https://firebase.google.com/docs/cloud-messaging/send-message?authuser=0#send-messages-to-multiple-devices

ctx := context.Background()
opts := []option.ClientOption{option.WithEndpoint("http://localhost:1234/")}
//opts = append(opts, option.WithoutAuthentication())
app, _ := firebase.NewApp(ctx, nil, opts...)
c, _ := firebaseApp.Messaging(ctx)

registrationTokens := []string{
        "YOUR_REGISTRATION_TOKEN_1",
        // ...
        "YOUR_REGISTRATION_TOKEN_n",
}
message := &messaging.MulticastMessage{
        Data: map[string]string{
                "score": "850",
                "time":  "2:45",
        },
        Tokens: registrationTokens,
}

br, err := client.SendMulticast(context.Background(), message)

Still get the error:

[2020-06-17T06:58:57.6171643Z] [TRACE] [] Firebase response: &{Success:false MessageID: Error:Request is missing required authentication credential. Expected OAuth 2 access token, login cookie or other valid authentication credential. See https://developers.google.com/identity/sign-in/web/devconsole-project.}

Verified that the request is hitting https://fcm.googleapis.com/batch

Culprit might be here:
https://github.com/firebase/firebase-admin-go/blob/master/messaging/messaging_batch.go#L233

@google-oss-bot
Copy link
Collaborator

This issue does not seem to follow the issue template. Make sure you provide all the required information.

@hiranya911
Copy link
Contributor

We can look into fixing it in a future release. But as usual I'd highly recommend against testing your code this way (see #219, #379). You should use blackbox testing with custom interfaces for all local testing. The RPC calls made by the SDK is an internal implementation detail, and might change in the future.

@jessejacksonanz
Copy link
Contributor

@hiranya911 to follow up this issue, blackbox testing shouldn't require you to modify the internals of an application to mock out external dependencies just for the purpose of testing, as this would be peering into the black box and writing code just to satisfy your tests (and running the service in a mode that does not match how it actually functions in a production environment).

You can indeed interface this out and run a service in a "mock mode", but that means introducing code into a code base that is purely there for test purposes, which is generally considered to be an anti-pattern.

@hiranya911
Copy link
Contributor

blackbox testing shouldn't require you to modify the internals of an application to mock out external dependencies just for the purpose of testing, as this would be peering into the black box

Certainly not. Developers should only ever rely on our public API. So they can define interfaces like the following:

type BatchSender interface {
  SendAll(ctx context.Context, messages []*messaging.Message) (*messaging.BatchResponse, error)
}

Now the messaging.Client provided in the SDK automatically becomes an implementation of the above interface, and you can easily create a fakeBatchSender for unit testing. If you need even more decoupling (which is another reason why people use interfaces with dependencies), you can define types around Message and BatchResponse too, but you don't have to.

However, when you run a mock FCM service, and point the SDK to that for testing (like the OP is attempting to), you're relying on a lot more than the SDK's public API. You're also relying on how the SDK communicates with the FCM backend, how the message marshaling/unmarshaling works, and how the backend itself responds to RPCs.

You can indeed interface this out and run a service in a "mock mode", but that means introducing code into a code base that is purely there for test purposes, which is generally considered to be an anti-pattern.

Putting third-party dependencies behind an interfaces is a well established pattern, and is even considered a best practice.

From the Go style guide: https://github.com/golang/go/wiki/CodeReviewComments#interfaces
Relevant quote from "Clean Code" and a discussion about this topic: https://softwareengineering.stackexchange.com/questions/298145/wrapping-third-party-library-is-best-practice

jessejacksonanz added a commit to jessejacksonanz/firebase-admin-go that referenced this issue Aug 6, 2020
hiranya911 pushed a commit that referenced this issue Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants