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

App development would be easier if group id's were required rather than optional in Catalog and Profile #1299

Open
4 tasks
fsuits opened this issue Jun 7, 2022 · 12 comments
Labels
enhancement Model Engineering An issue to be discussed during the bi-weekly Model Engineering Meeting User Story v2.0 future version of OSCAL

Comments

@fsuits
Copy link

fsuits commented Jun 7, 2022

User Story:

As an OSCAL application developer, coding logic would be simpler and clearer if group id's in Catalog and Profile groups were required rather than optional.

Goals:

Groups are present in Catalogs and in Profiles, and in each case the schema specifies a required title and an optional group_id. Since groups may be nested, and since the id is intended to allow direct reference to the specific group in a document, it makes sense for the id to be required rather than optional. This adds a small burden to the document creator - but would simplify application developers since they would not need to do special handling when the group id is not present - e.g. create an internal id on the fly during processing.

Dependencies:

No dependencies other than a change in the schema. There are presumably few use cases where the group id's are not already specified in a catalog or profile.

Acceptance Criteria

  • The schemas for Catalog and Profile are updated to require group id's in addition to group title.
  • All OSCAL website and readme documentation affected by the changes in this issue have been updated. Changes to the OSCAL website can be made in the docs/content directory of your branch.
  • A Pull Request (PR) is submitted that fully addresses the goals of this User Story. This issue is referenced in the PR.
  • The CI-CD build process runs without any reported errors on the PR. This can be confirmed by reviewing that all checks have passed in the PR.
@bradh
Copy link
Contributor

bradh commented Jun 7, 2022

As a consumer, you'll probably need to handle older data, which may not have the group id. So I'm not sure its really going to save effort. If it had been required from 1.0, then I can see the advantage, but it seems limited value now.

@hahsan-ti
Copy link

@fsuits should these IDs be required and unique or simply required?

@david-waltermire
Copy link
Contributor

@hahsanti The group ids are currently optional, but must be unique when provided.

One driver to allow groups without ids is to allow the catalog author to decide which groups are intended to be addressed by an id and which groups to not allow addressing in this way.

@fsuits This would be a backwards compatibility breaking change, so we could only consider this for an OSCAL 2.0.0 release.

@david-waltermire david-waltermire added the Model Engineering An issue to be discussed during the bi-weekly Model Engineering Meeting label Jun 7, 2022
@david-waltermire david-waltermire moved this from Needs Triage to Needs Discussion in Issue Triage Jun 7, 2022
@david-waltermire david-waltermire added the Discussion Needed This issues needs to be reviewed by the OSCAL development team. label Jun 7, 2022
@david-waltermire david-waltermire removed this from Needs Discussion in Issue Triage Jun 10, 2022
@rgauss
Copy link
Contributor

rgauss commented Jun 10, 2022

One could imagine the evolution of a REST API to include things like adding a new control to a group, i.e.:

POST /catalogs/{catalogId}/groups/{groupId}/controls

and that existing group could not be referenced without a group ID.

@aj-stein-nist
Copy link
Contributor

aj-stein-nist commented Jun 10, 2022

One could imagine the evolution of a REST API to include things like adding a new control to a group, i.e.:

POST /catalogs/{catalogId}/groups/{groupId}/controls

and that existing group could not be referenced without a group ID.

What would it mean for the API to support the following, given precious little details and just what-if-ing on the fly?

POST /catalogs/{catalogId}/controls: all controls returned, each control returned with a groupId being defined as a string or null. Whether or not the control is grouped (with or without a group with a given groupId, return isGrouped as true or false, so it would true if grouped but the group does not have an ID).

POST /catalogs/{catalogId}/groups: return all controls all IDs in array elements group per group in groups and assign "id": null for the property of a given group if no id is provided.

{ 
  "groups": 
    [
       {"id": "group1.a", "control-ids": ["g1a-1", "g1a-2"] },
       {"id": null, "control-ids": ["gnoid-3", "gnoid-4"] }
    ] 
} 

POST /catalogs/{catalogId}/groups/{groupId}/controls: probably what you'd expect, full control data properly grouped.

It seems that your proposed endpoints around {groups}/{groupId} are not implemented at all yet and the OSCAL catalog model could kind of supports my middle of the road approach. Is there a reason not to have a continuum with the current option group ID?

@rgauss
Copy link
Contributor

rgauss commented Jun 10, 2022

At a high level I think we want the REST API GET endpoints (which I think is what you mean above) to directly align with the OSCAL data models as much as possible, almost like a JSON path expression. With that in mind...

GET /catalogs/{catalogId}/controls: I would expect to return an array of only the 'top-level' controls in that catalog, i.e. with no group.

GET /catalogs/{catalogId}/groups: I would expect to return an array of the 'top-level' groups including the control objects (or subgroups) and other data like title, params, props, within each group.

GET /catalogs/{catalogId}/groups/{groupId}/controls: I would expect to return an array of just the control objects under that specific group.

When we look at the scenario of adding a new control to an existing group, we need to reference that group by a unique identifier. With an optional group ID there could be two groups with "id": null as you describe and there's no way to indicate which group to add it to.

It seems that your proposed endpoints around {groups}/{groupId} are not implemented at all yet and the OSCAL catalog model could kind of supports my middle of the road approach. Is there a reason not to have a continuum with the current option group ID?

Correct, not in the specification yet. We wanted to validate the approach with the community before going 'too deep' on the spec.

To be clear, I was just using REST APIs and this scenario as an example of where we might need to reference a group, which is a clear indicator that it needs an ID.

@aj-stein-nist
Copy link
Contributor

At a high level I think we want the REST API GET endpoints (which I think is what you mean above) to directly align with the OSCAL data models as much as possible, almost like a JSON path expression. With that in mind...

Makes sense. I (personally, not speaking for the team) can agree with that.

Correct, not in the specification yet. We wanted to validate the approach with the community before going 'too deep' on the spec.

Understandable and totally agree with the approach.

To be clear, I was just using REST APIs and this scenario as an example of where we might need to reference a group, which is a clear indicator that it needs an ID.

Yes, also completely reasonable and understandable. I think you did reframe the wording around this scenario.

GET /catalogs/{catalogId}/groups: I would expect to return an array of the 'top-level' groups including the control objects (or subgroups) and other data like title, params, props, within each group.

You described this better than I did. I know this is kludgey, but until 2.0 time frame, what would stop the API specification and developers from allowing {groupId} to be either a relevant UUID in the respective group object or perhaps the 0-based index of a group without a group ID (so if there are three groups and the second had no idea, you could do /catalogs/{catalogId}/groups/1/controls because you previously examined /catalogs/{catalogId}/groups and know that group has no explicit ID? OSCAL v2.0.0 is a long-time away (perhaps more than 3-6 months; I think Dave will have a better estimate). For that reason I ask if this is not a reasonable approach. Also worth examining (and I will follow up in the EasyDynamics/oscal-rest repo if it does come up, not here in this issue); what about other optional @id fields that impact API surface. Is this, even in catalog the only glaring example? If so, I am pleasantly surprised. :-)

@rgauss
Copy link
Contributor

rgauss commented Jun 13, 2022

You described this better than I did. I know this is kludgey, but until 2.0 time frame, what would stop the API specification and developers from allowing {groupId} to be either a relevant UUID in the respective group object or perhaps the 0-based index of a group without a group ID (so if there are three groups and the second had no idea, you could do /catalogs/{catalogId}/groups/1/controls because you previously examined /catalogs/{catalogId}/groups and know that group has no explicit ID?

The issue there is that we can't rely on order of JSON array elements in many implementations. It's entirely possible that the response of /catalogs/{catalogId}/groups/1/controls would differ from one request to the next.

I think the best short-term solution would be for the REST API to essentially layer additional constraints on the OSCAL models and state that group IDs are required/will be generated. We're doing something similar already by declaring that UUIDs must be scoped to the entire implementing system rather than just the OSCAL document.

what about other optional @id fields that impact API surface. Is this, even in catalog the only glaring example? If so, I am pleasantly surprised. :-)

I haven't looked in detail, but yes, I would assume there are other instances of this problem.

@aj-stein-nist
Copy link
Contributor

You described this better than I did. I know this is kludgey, but until 2.0 time frame, what would stop the API specification and developers from allowing {groupId} to be either a relevant UUID in the respective group object or perhaps the 0-based index of a group without a group ID (so if there are three groups and the second had no idea, you could do /catalogs/{catalogId}/groups/1/controls because you previously examined /catalogs/{catalogId}/groups and know that group has no explicit ID?

The issue there is that we can't rely on order of JSON array elements in many implementations. It's entirely possible that the response of /catalogs/{catalogId}/groups/1/controls would differ from one request to the next.

Just to be clear, I called it a kludge not because I thought it was an elegant and fully complete design. :-) I meant it as more of a workaround if the OSCAL model will not change short term around a required group ID.

Is the intent that your catalogs could be dynamically generated? If we look at them is static or semi-permanent database of control items, I had not really thought about your point about your point re serialization and deserialization in different runtimes and their implementations re arrays, so point well made!

@fsuits
Copy link
Author

fsuits commented Jun 16, 2022

With regard to the question of other optional id's, I believe the only optional id's are for groups, either in catalogs or profiles, and for parts. Parts are very granular and usually in a well defined context - so I can understand it would be somewhat overkill to require an id for them, while at the same time it might be convenient to allow one.

In contrast, groups are relatively coarse-grained and each control has a well defined path in a catalog that allows direct access. It will either be in the catalog's list itself, with no group at all, or follow a path of catalog->group->group->group->control->control->control - in the most general case. Without group id's this path becomes hard to specify - unless you assign group id's on the fly.

I created this issue from the perspective of a developer, since it is cleaner to rely on certain attributes to be present and not deal with situations of available/not_available and corresponding fallback logic. I'm not clear on the use case of wanting to prevent direct access to a group - and since groups have titles (that may not be unique) you can still use the title somewhat as a group id - at least for narrowing the search of where a control lives.

My preference as a developer is to have group id's be unique and required - while titles are completely flexible. This would help any part of code, or a REST interface, where the direct path to access the group or a control is needed without a search.

A specific need in trestle is the ability to map a catalog's group structure to a filesystem directory, where subgroups are simply named subdirectories based on the group id. Without a required id, temporary ones would be needed.

@david-waltermire david-waltermire removed the Discussion Needed This issues needs to be reviewed by the OSCAL development team. label Jun 24, 2022
@david-waltermire
Copy link
Contributor

We discussed this on the 6/24 model review and agreed that group identifiers need to remain optional to avoid a backwards compatibility breaking change. This can be revisited in OSCAL 2.0.0.

@aj-stein-nist aj-stein-nist removed this from the Future milestone Jul 27, 2023
@Arminta-Jenkins-NIST Arminta-Jenkins-NIST added the Aged A label for issues older than 2023-01-01 label Nov 2, 2023
@Arminta-Jenkins-NIST Arminta-Jenkins-NIST added v2.0 future version of OSCAL and removed question Aged A label for issues older than 2023-01-01 labels Nov 30, 2023
@Arminta-Jenkins-NIST
Copy link
Contributor

At the 11/30 Triage Meeting: The team decided to pursue this ticket in the next version of OSCAL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Model Engineering An issue to be discussed during the bi-weekly Model Engineering Meeting User Story v2.0 future version of OSCAL
Projects
Status: Todo
Development

No branches or pull requests

8 participants