Normalize collection_uuid and environment_uuid parameters to collectionUuid and environmentUuid

[martinlocklear]

Does exactly what it says on the tin - just a straightfoward rename. This is part of a series refactors in this area of code to make things a little more consistent.

Checklist:

• Tests passing (make test-community)?
• Lint passing (make lint this requires golangci-lint)?

[casey-tran]

For consistency purposes, do we want to have collectionId be collectionUid?

[martinlocklear]

I'm pretty sure that this is a situation where this is not a uid, but rather an id - but I'm not always sure what the intent was. Do you read this situation differently?

[casey-tran]

The creation of the url variable a few lines down at 359 has the collection_uuid/collectionId used to create the endpoint for the collection as a uid in the JSON response. I can see how collectionId can make sense as well given the documentation for Collections.

[camgunz]

TIL postman's API docs are in postman itself, which is a super heavy SPA 😿

[camgunz]

I dug a little and I think collection IDs can be random strings--I think you assign a collection an ID when you create it. Refs:

Collection
Which inherits from: ItemGroup
Which inherits from: Property

[martinlocklear]

Thanks so much for digging into this! So, the change in this PR should be non-functional. Just a name change of two variables. However...

Y'all's comments are making me think that either I'm missing something (likely), or I haven't explained something well enough (potentially also likely), so I took the time to make the following write up so (at least) I can expose some of my ignorance.

Note - The following is my understanding right now. I'm definitely feeling more of the second picture than the first.

Postman World

In Postman world ™️ every item has both an ID and a UID field. The ID field usually (but maybe not always) contains a string that is a well formed UUID (as in the data type). The UID field never is a well formed UUID (since it has the user id just concatenated to the front).

From the Postman API Docs (in the introduction):

IDs and UIDs
All items in Postman, such as collections, workspaces, and APIs, have IDs and UIDs:

• An ID is the unique ID assigned to a Postman item. For example, ec29121c-5203-409f-9e84-e83ffc10f226.
• The UID is the full ID of a Postman item. This value is the item's unique ID concatenated with the user ID. For example, in the 12345678-ec29121c-5203-409f-9e84-e83ffc10f226 UID:
  • 12345678 is the user's ID.
  • ec29121c-5203-409f-9e84-e83ffc10f226 is the item's ID.

Note for later: It says the phrase "the UID is the full ID of a Postman item. [emphasis mine]

It appears that the ID field and the UID field are functionally interchangeable, but the documentation seems to always refer to the ID as the parameter required to specify an object in a GET API request. (Example, the endpoint to GET a workspace refers to the path parameter as an ID).

Note for later: In that API endpoint it notes that "name" and "uid" are both deprecated in the response objects, which seems to imply (to me at least) that Postman realized that having multiple IDs for objects is a tricky biscuit and they're potentially phasing them out.

We have deprecated the name and uid responses in the following array of objects:

• collections
• environments
• mocks
• monitors
• apis

Truffle World

In Truffle world, things are slightly less consistent.

Postman Source/Client

It appears that we sometimes use the convention {object_type}_uuid to refer to an item's ID field (presumably because it usually contains a UUID), e.g. collection_uuid. Sometimes we use the convention {object_type}_id to refer to the same bit of data from Postman, e.g. collection_id. Sometimes we refer to an item's UID field using the convention {object_type}_uid, e.g. collection_uid, and sometimes we refer to the same bit of data from Postman using the convention {object_type}FullID, e.g. CollectionFullID.

That's just naming though. In at least one place (and likely others implicitly) we use a field labeled FullID to optionally store either a UID or an ID. It's not clear to me at this point why or when this happens.

Postman gRPC interface

Here we are passing things around in fields called collections or workspaces, but those fields are most certainly not carrying either. They seem likely to be carrying either a UID or an ID for the referred to object, but I'm not sure yet.

Next steps

1. Normalize all the naming everywhere using the following strategy:
   • If the field carries an item ID given to us from Postman, normalize the suffix to either Id or _id as appropriate.
   • If the field carries an item UID given to us from Postman, normalize the suffix to either Uid or _uid as appropriate.
     • This includes fields currently labeled without prefixes, but carrying just an ID or a UID
   • If the above is not possible, litter the declaration and any usages of the field with warning comments.
2. Split any field that might be carrying either a UID or an ID into two fields that are precisely carrying either one or the other.
3. When doing 1 and 2:
   • Watch out for any mismatched assignments (i.e the value of a field labeled uid being stuffed into a field labeled id)
     • I've identified at least one of these already.
   • Watch out for any situation where union field (step 2 above) was implicitly creating union fields downstream
   • Notice if any of the above explains the failures we're seeing

Once 1 through 3 are finished:

4. Remove usages of uid entirely, if possible. I don't see the benefit of carrying around two different identifiers for the same item, and it certainly makes things confusing. I'd probably leave the place where we are capturing it from the API response, just to leave a really visible "thar be dragons" tombstone.

[martinlocklear]

@casey-tran - Upon reconsidering this PR, I think I see another source of confusion. In this PR I'm changing two variable names. I believe that collection_uuid was intended to be used as an ID, and environment_uuid was intended to be used as a UID. I'm inferring this from the callers of the functions.

However, there's some inconsistency already related to this (see this PR for an example). The callers of the functions I'm changing are also inconsistent, upon review.

All of that is a big part of why I'm planning on eventually ripping out all uses of UID in area of code. But thanks again for asking the question! Among other things, it forced me to organize my thoughts better and revisit some assumptions that I had made along the way.

Not sure if all of this alleviated your concerns though - please let me know if I've missed something

[martinlocklear]

As it turns out, Postman isn't consistent in their documentation, or in their implementation. Sometimes ID strictly means UID. More research incoming

[casey-tran]

Thanks for all that research @martinlocklear. From reading through the information that you've put out, I'm inclined to now suggest that environmentUid be called environmentId since functionally for Postman API calls you can use either UID or ID as you pointed out. And there's the fact that Postman will be deprecating UID in the response as you've pointed out as well. So maybe this can be the first step towards eliminating references to Postman UID in the code.

[nabeelalam]

Hey @martinlocklear, I was wondering what convention are following to normalize the variable names here.
Seems according to Google's Go styleguide, the UID initialism would be in all caps, meaning the variable would be environmentUID.
There's an example of this usage in this file:

trufflehog/pkg/sources/postman/postman_client.go

Line 311 in 48ce290

func (c *Client) GetWorkspace(ctx context.Context, workspaceUUID string) (Workspace, error) {

[martinlocklear]

@nabeelalam - Great question!

The strategy I'm following here is to treat initialisms as words themselves, subject to the same casing strategy.

Why the change?

Right now it really seems like naming has gotten us into a bit of a mess (see this comment for a pretty decent summary of my understanding of the situation), so I'm going through and doing my best to tidy things up.

To be honest, I think this is one of several things that Google gets wrong (luckily they included a bit of a relief valve in their style guide). It's possible even that this style influenced the confusion here (visually distinguishing UUID from UID is maybe more error prone than distinguishing Uuid from Uid? Maybe plausible? I dunno - but certainly would be useful for my argument right now if it were true 😅 )

At the moment, I'm definitely planning to update the example you referenced here (workspaceUUID will become workspaceUid - note the missing "u"). Ideally, it'll eventually be removed entirely as I remove the use of UIDs entirely from our Postman source. Assuming, of course that I haven't missed something important. That change isn't included in this PR because I'm trying to keep this series of PRs individually small to facilitate review.