LiveRamp / reslang

A language for describing resource-oriented APIs & turning them into Swagger or resource diagrams. Oriented around the concepts we want to expose in the APIs.
Apache License 2.0
23 stars 7 forks source link

Don't include "Bulk" in operations names for resource-level actions #147

Open ChristianHansen opened 3 years ago

ChristianHansen commented 3 years ago

Description

Do not automatically include "Bulk" in the operation name for resource-leve actions. I believe this is the line causing that to happen.

Why it's valuable

Honestly, I might be fighting against the spirit of the feature here, but it seems like we shouldn't assume that all resource-level actions are necessarily bulk actions. A bit of a funky "real-world" example is this action which is meant to create a single resource using a property (LIR ID) that we don't want to live on the resource itself, but need to use to create the initial resource for the time being.

If I am fighting the spirit of the language and there's another approach more in line with the spirit of reslang and the API standards, I'm open to that.

rupertchen commented 3 years ago

Related slack thread: https://liveramp.slack.com/archives/CPBAEKS9X/p1608346245035100

Here's what I think is the cause:

  1. The string “bulk” or “resource-level” gets placed into the bulk member.
  2. Actions where bulk is truthy get prefixed with “Bulk”.

A similar piece of logic exists in formSingleUniqueName (line 721). It's unclear to me whether only one or both should be changed. I suspect they need to be kept in sync because they from the grammar, actions are the only thing to set the bulk member.

rupertchen commented 3 years ago

After some more digging, it seems to be that "bulk" and "resource-level" are currently equivalent to each other. I see no references to the value of the bulk member, only truthy checks. Swapping "bulk" for "resource-level" likewise results in no change to the generated OpenAPI spec.

I'd like to explicitly propose for this issue that resource-level only alter the endpoint (as it does today) but does not have any mention of "bulk" in the resource name or documentation.

Now seems the time to change this as there are only two uses of "resource-level" right now and both are still in draft phases.

  1. taxonomy
  2. field-mapper
njaczko commented 3 years ago

Hi @rupertchen , I chatted about this with the rest of the API team. Our proposed solution is as follows:

rupertchen commented 3 years ago

I am interested in having that third bullet, so I'm happy to wait for that to be available.

I don't understand why resource-level would mean the same thing as having no modifier. Is the term being redefined to be "resource instance level"? Perhaps because I'm reading "resource level" as being like "class level" in Java, this caught me off guard.

If you're still fielding names, what about: bulk, singleton, and resource-level? For better or worse, singleton's already a keyword.

njaczko commented 3 years ago

I don't understand why resource-level would mean the same thing as having no modifier. Is the term being redefined to be "resource instance level"? Perhaps because I'm reading "resource level" as being like "class level" in Java, this caught me off guard.

We happened to read resource-level contrarily: like resource instance level instead of like class level. If most devs will read it like class level I'm happy with the solution that you originally proposed:

I'd like to explicitly propose for this issue that resource-level only alter the endpoint (as it does today) but does not have any mention of "bulk" in the resource name or documentation.

@forestgagnon @cjea , what do you guys think?

rupertchen commented 3 years ago

@njaczko

I don't know if my interpretation will be what others have. I came to mine "in reverse" by looking at what was generated and what we used and trying to understand what reslang might have meant.

Reviewing this now I had some additional thoughts (some of which feel contradictory):

Ultimately, I think I'll be fine with whatever you all decide to go with.

njaczko commented 3 years ago

Thanks, @rupertchen . Seems like the API team needs to spend a little more time thinking about the best solution. Since it's not the quick fix I had hoped for and the priority is rather low I'm not exactly sure when we'll get to this. Soon, I hope, but I'd like to be realistic and not make any promises.

forestgagnon commented 3 years ago

@sirishalal I marked this as low priority, but that's because we can tolerate noise in the documentation for internal docs. If these docs are to be exposed externally, the instances of "Bulk" being added in nonsensical cases would be more problematic.