Add data to schema to indicate Flag enumerations

[pylaligand]

A group capability is defined as such:

"GroupsV2.Capabilities": {
      "format": "int32",
      "enum": [
        "0",
        "1",
        "2",
        "4",
        "8",
        "16",
        "32",
        "64"
      ],
      "type": "integer",
      "x-enum-values": [
        . . .
      ]
    },

When using the GroupV2.GetGroup endpoint, the returned group has within its features a capabilities property set to 31, which is not a valid enum value.

It looks like the property is not a capability, but a union of capabilities (in my case, 1, 2, 4, 8, and 16). If that's the case, then instead of the capabilities of the GroupsV2.GroupFeatures type being directly a GroupsV2.Capabilities, it should probably be defined as an array of capabilities:

"capabilities": {
  "type": "array",
  "items": {
    "$ref": "#/definitions/GroupsV2.Capabilities"
  }
},

instead of:

"capabilities": {
  "$ref": "#/definitions/GroupsV2.Capabilities"
},

[vthornheart-bng]

Hmm, we should add some metadata to indicate this. Some of our enumerations are Flag enumerations, and this is one of them - 31, as you surmised, indicates the bitwise OR of capabilities 16, 8, 4, 2, and 1.

I'm going to turn this into an enhancement request - we need to be able to convey when certain enumerations are Flags. For the time being as a workaround, you can safely infer that any Enumeration defined similarly to this one (where values are powers of two) are meant to be used as bitwise flags.

[pylaligand]

Thanks for the clarification. I'm worried thought that metadata alone won't be enough, unless it is standard enough that code generators are able to handle it. If GroupsV2.Capabilities remains the same enum, then 31 is not a valid value.

Another way to work around this would be to include all possible values in the enum, and add the flag definitions (for 0, 1, 2, 4, 8, etc...) as metadata. This way code generators will correctly handle all possible values. This is not very elegant but it does the job :)

[vthornheart-bng]

Good call - I'll look into it when I get time to loop around to this as well. See if there's already a defined standard that generators should know about for handling flag enumerations. If there's not, we'll come back to the issue of what to do about it!

[pylaligand]

I looked at the specs and there's nothing about flag enumerations.

[vthornheart-bng]

Thanks for taking a look into that! Hmm, sounds like we've got some choices to make then. Our options appear to be:

1. Change the API itself to not return flags enumerations. This will not happen: we have too much code that's dependent on this functionality, and our choice to use flags enumerations was intentional for a number of reasons.

2. Enumerate every possible combination. This seems infeasible to me, as we have some enumerations, such as ApplicationScope, that already exceed 1000 possible combinations between their bitfields. That doesn't feel like a scalable option: for instance, I could certainly picture ApplicationScope being added to, which will result in exponential growth of the "every possible combination pre-enumerated" approach. We will quickly regret this decision if we decide to go down this path for a quick fix.

3. We begin representing flags enum fields as straight integers in the spec, and rely on you to do your own mapping in your code. With the inclusion of extension properties, you could write your own code to automate that mapping.

(3) appears to be the only feasible option at the moment. In the long term, I'd like to petition OpenAPI about making flags enumerations part of the standard: I can't imagine that we're the only web service out there returning bitfields. But even if that was approved, adoption of whatever standard gets made for it would be far down the road. (3) seems to be the most pragmatic and scalable option at the moment.

As this change would affect those of you building clients and scrapers,

[floatingatoll]

Cut off at “scrapers,”?

…

On Sep 20, 2017, at 10:32, Vendal Thornheart ***@***.***> wrote: Hmm, sounds like we've got some choices to make then. Our options appear to be: Change the API itself to not return flags enumerations. This will not happen: we have too much code that's dependent on this functionality, and our choice to use flags enumerations was intentional for a number of reasons. Enumerate every possible combination. This seems infeasible to me, as we have some enumerations, such as ApplicationScope, that already exceed 1000 possible combinations between their bitfields. That doesn't feel like a scalable option: for instance, I could certainly picture ApplicationScope being added to, which will result in exponential growth of the "every possible combination pre-enumerated" approach. We will quickly regret this decision if we decide to go down this path for a quick fix. We begin representing flags enum fields as straight integers in the spec, and rely on you to do your own mapping in your code. With the inclusion of extension properties, you could write your own code to automate that mapping. (3) appears to be the only feasible option at the moment. In the long term, I'd like to petition OpenAPI about making flags enumerations part of the standard: I can't imagine that we're the only web service out there returning bitfields. But even if that was approved, adoption of whatever standard gets made for it would be far down the road. (3) seems to be the most pragmatic and scalable option at the moment. As this change would affect those of you building clients and scrapers, — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[vthornheart-bng]

Whoops! I meant to say -

As this change would affect those of you building clients and scrapers, I want to give advance notice of this change. I'm pretty resolved at this point to go down the path of representing flags enums as integers and letting users figure out in their own code how to interpret said enums. The relationship between the use of the flags enum as an integer and the enumeration definition will be represented by another extension property, and I'll retain exposing the enum definitions themselves so that people can leverage them.

Is anyone actively impeded by this? @pylaligand , is the way this is defined actively causing bugs in your application? I'm trying to get a sense of the urgency of this problem relative to - for example - bugs that are on my plate for the API itself. Right now I'd like to get through more of the API bugs that have been discovered before I worry about fixing bugs in or improving on the spec file, but I'd also not like to impede anyone being able to make things with the API if the spec is actively blocking them. If you can, work around it for now - if your chosen language will allow it, force the value coming down to be cast to an integer and handle it manually for the time being. We will loop back around to this.

[pylaligand]

I am not blocked at the moment as I use my own copy of the API file to work around other issues such as #149. I think what you propose with (3) is very reasonable, especially if this comes with proper documentation. Thanks for looking into this.

[vthornheart-bng]

Good deal, phew! Glad to hear it. And thank you for bringing it to my attention - I took flags enumerations for granted! My C# is leaking out. ;)

[vpzed]

IMO as long as the spec is updated to remove the enum, then it would be a step on the right direction. An enum means that it can only have the enumerated values so any code that is doing validation of input/output against the spec would reject a value of 31 for example. If you changed it to a straight integer and put how it works in the description or something and like you said left it up to the user to code correctly that would at least keep it from breaking codegens or validation of input/output.

[pylaligand]

Any update on this? This is a frequent problem (e.g. with DestinyGameVersions, and now DestinyRecordState).

[Yogarine]

I'm currently running into this problem as well. Core generated by OpenAPI Generated attempts to validate the enums returned by the API and throws an Exception.

Any update when this is going to be defined as an integer in the spec?

[vthornheart-bng]

I just got a pull request to fix this - which while I can't take directly as it's our spec generator that needs the fix, it does bring this long-backlogged bug back to life.

I can add this to the Winter Wishlist if this is a sufficient enough burden that you would want it prioritized over other issues! Let me know, and see the bottom of the thread here for the other issues in the Winter Wishlist. I'd be glad to add it in there if this is causing a lot of pain:

#756

[vthornheart-bng]

To note, I'd place this one in the "Feasible without significant changes/with known cost of changes" which makes it very likely to be fixed if it is something that people feel is worth bumping down others for the sake of fixing.

[Yogarine]

Ah, my bad. I figured it would be generated. I created the fork mostly to have a patched spec file that I could generate my SDK from and decided to create a PR to are least have a reference.

I didn’t realize the Winter Wishlists existed. In all honestly: posting it as an issue is not very visible. Maybe you should consider creating something of a mailing list.

Anyway, I’ve added it to the wishlist. Thanks for taking the time to reply!

[vthornheart-bng]

No prob, and I appreciate your input into all this! I'll go ahead and add it into the winter wishlist!

Indeed, I could see adding a mailing list or some other solution when I want people to take active participation in something upcoming. I need to look into what options are around for that... ideally I'd like it to be something we have an active/permanent record about so people can look back at the history (which is where Issues are handy, but indeed not easy to find).

Perhaps it could be as easy as having something at the top of our readme file that links to current "issues that need your immediate attention/response" that then links to an issue for discussion. Hmm. I'm open to suggestions if anyone has any!

[Yogarine]

Ideally I'd like it to be something we have an active/permanent record about so people can look back at the history (which is where Issues are handy, but indeed not easy to find).

Most mailing list tools also offer an archive to read old mailings. Alternatively you can add a blog (at developer.bungie.com for example) with an RSS feed.

Perhaps it could be as easy as having something at the top of our readme file that links to current "issues that need your immediate attention/response" that then links to an issue for discussion.

That most certainly would give the wishlist more visibility, so that is probably a good immediate fix.

Anyway, this is all off-topic, so I created an issue to discuss this here: #836

[vthornheart-bng]

Many thanks!

[vthornheart-bng]

TFS 746596

[vthornheart-bng]

Okay, so this'll be ready for January - flags enum values are going to become integers, and references to enums will get a new extension property (including flags enums), "x-enum-reference". This way even if we're treating them as integers in the spec, you can write a handler for this new extension property to pull the possible values that it can be.

For enum definitions themselves, there will be a new extension property "x-enum-is-bitmask" - if it exists and is true, the enum is a bitmask/flags enumeration. I'll also return this on properties that use these types, for convenience.

For example, the "state" property on DestinyItemComponent would, with this change, look like:

"state": {
            "type": "integer",
            "description": "A flags enumeration indicating the transient/custom states of the item that affect how it is rendered: whether it's tracked or locked for example, or whether it has a masterwork plug inserted.",
            "format": "int32",
            "x-enum-reference": {
              "$ref": "#/components/schemas/Destiny.ItemState"
            },
            "x-enum-is-bitmask": true
          },

[vthornheart-bng]

This should be fixed as of today's deployment (1/29)!

[vthornheart-bng]

Though to note, I've not yet deployed the documentation - that will be coming within the hour.