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

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

Open
ChristianHansen opened this issue Oct 16, 2020 · 8 comments
Open
Assignees

Comments

@ChristianHansen
Copy link

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.

@ChristianHansen ChristianHansen added the enhancement New feature or request label Oct 16, 2020
@rupertchen
Copy link

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
Copy link

rupertchen commented Dec 21, 2020

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
Copy link
Contributor

njaczko commented Dec 23, 2020

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

  • leave the bulk modifier unchanged
  • change resource-level to mean the same thing as including no modifer (ID parameter in the path)
  • introduce a new modifier (yet to decide on a name) that is the same as bulk, but it does not include the pesky Bulks in the OpenAPI

Sound good? I hope to have a PR up next week.

I'll coordinate with the taxo and field-mapper folks about updating their use of resource-level.

@njaczko njaczko self-assigned this Dec 23, 2020
@rupertchen
Copy link

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
Copy link
Contributor

njaczko commented Dec 24, 2020

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
Copy link

@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):

  • I like the idea that there is a reslang term for the "default" since giving the behavior a name clarifies all the other options even it never needs to be used.
  • I feel like the internal bulk property is misnamed now that there are three possible states. I get the impression that resource-level's current behavior was a bug and it was meant to be boolean the way "singleton" is today. Perhaps the property on the action definition is better named "number" or "singleton" because it's like the singleton portion of a resource definition with the added "bulk" option which behaves like "singleton", but with additional automated docs.
  • My early suggestion of bulk, singleton, resource-level now seems strange to me because for resource and subresource "singleton" doesn't have a resource-level keyword and it doesn't feel like it needs.

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

@njaczko
Copy link
Contributor

njaczko commented Dec 31, 2020

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
Copy link
Contributor

forestgagnon commented Aug 18, 2021

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants