Missing copyright notice here and elsewhere

Good catch! I had a script I wrote that would automatically add it to any missing files, but it doesn't auto-run. We should probably have some sort of check regarding that. Not anything that's enforced, since there are way too many exceptions, but maybe like a git hook or gha that looks at files added and lets you know if any of interest are missing copyright notices (.kt,.java,etc.,).

There is one, and it's correctly reporting this :-)

https://github.com/firebase/firebase-android-sdk/actions/runs/3917153439

LOL, well I'll be paying more attention to that one for sure

What does the P suffix stand for?

Protobuf, implying that these data classes are representations of gRPC related data (that is, they're raw classes that shouldn't be used directly, and are intended to only be used by the network layer). I'm not quite sure where I picked this up from, but I've used this naming convention by habit in all of my personal gRPC projects. Helps more easily difference between the network and application layer.

Although if you're against it, I'm fine with removing it:)

Just to be consistent with the rest of our code, I'd rathe remove it.

That being said, the discussion of the value and usage of suffixes in code naming is a long and interesting one for the future :-)

+1 to removing the P suffix. If you really want to add a suffix for clarity, perhaps just fully putting Proto instead? Unless the P shortening is a Kotlin convention.

If this is an OIDC credential, would it make sense to call the class that? Not sure if implicit credentials is the right naming.

I would be comfortable with that, although the property name would still be implicitCredentials in the API call, because that's what the API expects.

I can't link it here since it's an internal link- but if you look at the bug linked in this PR (referenced in the OP), and go to the first link in the comments of that bug, you can see all the proto defs for the API. These definitions and their names are derived from that- since these are all just data classes for the API- intended to be encoded to JSON via @Serializable

Actually, I forgot we can specify a @SerialName for encoding and decoding, so we would call it something else but it gets translated to implicitCredentials when it's sent. But seeing as this is ONLY for the API, and shouldn't be used beyond that- I think it's totally fine keeping it as is. Thoughts?

This is an scenario where more details help :-) You could change the first line of the description to "Credentials returned by an OIDC implicit flow" Then the name would sound more fitting. No need to use @SerialName

what do you mean by a proto type in this case?

As in it's a data class used for communicating via protobuf APIs (the backend for auth echange). Although I do think the phrasing gives off the wrong impression. Thoughts on this instead:

 * Not to be confused with [AuthExchangeToken][com.google.firebase.authexchange.AuthExchangeToken],
 * this is intended only for network requests.

Or maybe just removing it entirely?

+1 your suggested rewording.

Is this a header or the actual request?

here and below

So nothing here actually implements the requests themselves. These are all just headers, or type defs, of what the API expects. Skeletons you could say. Actual implementation and utilization of these classes will be elsewhere in the SDK.

The API is currently defined in protobuf in G3, and all these classes provide a type-safe kotlin variant of the API as it is defined. Additionally, we can use kotlin's serialization, and immediately encode classes to JSON requests for the backend, and vise versa for JSON responses from the backend.

Just a little quick example (psuedo code):

data class GetPersonRequestP(val name: String)
data class GetPersonResponseP(val person: Person?, val error: String?)

data class Person(val name: String, age: Int, phoneNumber: String) 

fun getPerson() {
  val myQuery = GetPersonRequestP("Rodrigo")

  val response: GetPersonResponseP = googleServer:GetRequest<GetPersonRequestP>(myQuery)

  response?.person?.let {
    println("Rodrigo is ${it.age} years old!")
  }
}

All subclasses of Request are definitions for what individual API endpoints expect in their request call.

All subclasses of Response are definitions for what API endpoints return in their response to request calls.

Anything else is just extra data types that either need to be arguments in a call to a certain API, or are apart of a response.

Does that answer your question? Or did I misunderstand what you were asking?

All below are either Request or Response subclasses, and they are named as such, but the doc says "headers" which is different. A header is metadata about the request, but these classes contain the actual content of the requests/responses.

All below are either Request or Response subclasses, and they are named as such, but the doc says "headers" which is different. A header is metadata about the request, but these classes contain the actual content of the requests/responses.

They contain the content sure, but only in the sense that it's a data class and doesn't have any content. I think C headers- where data types and function signatures are declared in header files, and the source file implements said methods.

Although, what do you think would be a better name? We could also always just split it up into two separate files; request.kt and response.kt.

+1 to "request header" implying network request headers, for example the x-firebase-appcheck header.

Though I'm not sure I understand what you mean when you say the actual implementation of these classes will exist elsewhere -- from my understanding of a Kotlin data class, it is essentially like a struct, which is fully instantiable (i.e. it's not like a Java interface where you need to implement the interface before it can be used). Also, are these data classes not implementations of the Request interface already?

from my understanding of a Kotlin data class, it is essentially like a struct

You're right on the money

what you mean when you say the actual implementation of these classes

Implementation was probably the wrong phrasing, I more-so meant the usage of these classes occur elsewhere- and this isn't intended to provide any logic

Also, are these data classes not implementations of the Request interface already?

They are, I just meant splitting the files for naming & organization sake

I see -- in terms of naming clarity, I think you can just remove the term "header" from the documentation and leave the rest of the comment the same.

+1

+1 to removing the P suffix. If you really want to add a suffix for clarity, perhaps just fully putting Proto instead? Unless the P shortening is a Kotlin convention.

+1 to "request header" implying network request headers, for example the x-firebase-appcheck header.

Though I'm not sure I understand what you mean when you say the actual implementation of these classes will exist elsewhere -- from my understanding of a Kotlin data class, it is essentially like a struct, which is fully instantiable (i.e. it's not like a Java interface where you need to implement the interface before it can be used). Also, are these data classes not implementations of the Request interface already?

The ExchangeOidcTokenRequest can take either implicit credentials or auth code credentials (see the API proto). Is there a way to represent this as a oneof?

For sure, although do we use it on our end? I initially had the following comment:

/**
 * While this isn't the only OIDC flow that the API supports, it's the only flow that the mobile SDK
 * supports- so there is no base class for credentials.
 */

Because I was under the impression we don't utilize AuthCodeCredentials on the SDK?

The auth code OIDC flow is the "headful" flow (detailed here).

The token field is unnecessary as part of the request object (here and in all the requests). In the backend, the token field is extracted from the request URL.

Ahh I see. Is it the key param, or is it actually called token?

What do you mean by key param? For additional context -- here is the request path. Note the token= portion of the request path -- that is where the token field is populated from.