Ability to add and update SSP Impl Req #94

rgauss · 2022-05-12T17:53:24Z

Created an OscalSspImplReqMarshallerImpl
Updated IterableAssemblyClassBinding to define a list of
secondaryRootObjects that will 'trick' the deserializer to allow as
root objects
Updated BaseOscalObjectMarshallerLiboscalImpl to be able to create
deserializer with IterableAssemblyClassBinding
Updated SspController and tests

Closes #57
Closes #58

- Created an `OscalSspImplReqMarshallerImpl` - Updated `IterableAssemblyClassBinding` to define a list of `secondaryRootObjects` that will 'trick' the deserializer to allow as root objects - Updated `BaseOscalObjectMarshallerLiboscalImpl` to be able to create deserializer with `IterableAssemblyClassBinding` - Updated `SspController` and tests

laurelmay

Okay so love this. I started to take a look at doing this and got a little put off (overwhelmed) by the fact this was the first time we're really handling any sort of non-universal "inner" elements (because like we play with UUID and Metadata but everything root has one of those). So not only is this cool because I want this feature but I think this is cool because it's a really good starting point for all the features to come which is really exciting.

Nothing here is concerning enough to me to block merging but I do want to ask a few questions to make sure I understand what's going on before I fully approve (and of course, while I was making comments anyway, I made fun of Java once or twice).

...mons/src/main/java/com/easydynamics/oscal/data/marshalling/IterableAssemblyClassBinding.java

oscal-rest-service-app/src/main/java/com/easydynamics/oscalrestservice/api/SspController.java

...mons/src/main/java/com/easydynamics/oscal/data/marshalling/IterableAssemblyClassBinding.java

oscal-rest-service-app/src/main/java/com/easydynamics/oscalrestservice/api/SspController.java

...-app/src/main/java/com/easydynamics/oscalrestservice/api/OscalControllerExceptionAdvice.java

laurelmay

See #95 for issues around spec compliance. It was a little too cumbersome to add as suggestions directly in this PR.

Separate the implementation of the POST endpoint (that adds a new Impl Req) and the PUT endpoint (that updates existing Impl Reqs) into two functions. This is done to match the API spec which specifies different parameters for the two endpoints; the POST endpoint does not need the Impl Req ID.

Update the tests to accomodate the changes to the POST SSP Impl Req endpoint's parameters

Resolve linter complaints on SSP Impl Req implementation and tests

hreineck

Awesome! Works great in my local Docker deployment.
Only a couple notes.

hreineck · 2022-05-14T13:48:01Z

oscal-rest-service-app/src/main/java/com/easydynamics/oscalrestservice/api/SspController.java

+          .orElse(null);
+    }
+    if (existingImplReq != null && isCreateOnly) {
+      throw new OscalObjectConflictException("Implented Requirement already exists");


Suggested change

throw new OscalObjectConflictException("Implented Requirement already exists");

throw new OscalObjectConflictException("Implemented Requirement already exists");

oscal-rest-service-app/src/main/java/com/easydynamics/oscalrestservice/api/SspController.java

hreineck · 2022-05-14T15:47:46Z

oscal-rest-service-app/src/main/java/com/easydynamics/oscalrestservice/api/SspController.java

@@ -23,13 +35,17 @@
 @RequestMapping(path = "/oscal/v1")
 @RestController
 public class SspController extends BaseOscalController<SystemSecurityPlan> {
+  private final Logger logger = LoggerFactory.getLogger(this.getClass());
+  private final OscalObjectMarshaller<ImplementedRequirement> oscalSspImplReqtMarshaller;


Suggested change

private final OscalObjectMarshaller<ImplementedRequirement> oscalSspImplReqtMarshaller;

private final OscalObjectMarshaller<ImplementedRequirement> oscalSspImplReqMarshaller;

Maybe change this to match the naming convention of "Impl Req" used elsewhere

hreineck · 2022-05-14T17:30:57Z

oscal-rest-service-app/src/main/java/com/easydynamics/oscalrestservice/api/SspController.java

+  @PostMapping(value = "/system-security-plans/{id}/control-implementation/"
+      + "implemented-requirements/{implementedRequirementId}",


Per the API spec, the POST request does not include the implementedRequirementID as a parameter.

Kind of annoying because the ID validation cannot happen in the marshaller - the functionality probably needs to be separated between the POST and PUT.

hreineck · 2022-05-14T17:34:23Z

Per my comment about the POST endpoint parameters, see the commits I just pushed for a potential way to separate out the POST and PUT endpoints. If we want a different way or to change the API spec I can revert

Per the specification, we must return the _new_ implemented requirement object; not the entire SSP. We can add somewhat more generic versions of the existing `makeXxxResponse` methods for this use case (and we can leave the old implementations as more convenient wrappers for the root object types).

Only return the new implemented requirement

laurelmay

Thanks for your changes! This looks good as far as I can tell. We still need to see why the Viewer doesn't update after a PUT but right now I'm not sure anyone else has reproduced that behavior and it also feels more likely to be a viewer bug.

rgauss · 2022-05-18T12:25:27Z

Thanks for your changes! This looks good as far as I can tell. We still need to see why the Viewer doesn't update after a PUT but right now I'm not sure anyone else has reproduced that behavior and it also feels more likely to be a viewer bug.

@kylelaker, see EasyDynamics/oscal-react-library#396

rgauss requested a review from a team May 12, 2022 17:53

laurelmay reviewed May 13, 2022

View reviewed changes

Improved secondaryRootObjects handling

8a66b17

rgauss requested a review from laurelmay May 13, 2022 12:04

laurelmay approved these changes May 13, 2022

View reviewed changes

laurelmay requested a review from a team May 13, 2022 21:04

laurelmay suggested changes May 14, 2022

View reviewed changes

hreineck added 3 commits May 14, 2022 12:34

Update Tests POST SSP Impl Req

c1574b5

Update the tests to accomodate the changes to the POST SSP Impl Req endpoint's parameters

Fix Linter Issues

926cae5

Resolve linter complaints on SSP Impl Req implementation and tests

hreineck suggested changes May 14, 2022

View reviewed changes

Kyle Laker and others added 3 commits May 14, 2022 21:19

Merge pull request #95 from EasyDynamics/suggestion/update-ssp-impl-req

0654bd1

Only return the new implemented requirement

Spelling fixes

0ca3851

rgauss requested review from laurelmay and hreineck May 17, 2022 10:48

laurelmay mentioned this pull request May 17, 2022

Provide implementation guidance on handling UUIDs in POST/PUT EasyDynamics/oscal-rest#51

Closed

laurelmay approved these changes May 18, 2022

View reviewed changes

rgauss requested a review from a team May 18, 2022 19:40

hreineck approved these changes May 19, 2022

View reviewed changes

rgauss merged commit f771520 into develop May 20, 2022

rgauss deleted the feature/update-ssp-impl-req branch May 20, 2022 13:09

laurelmay mentioned this pull request May 20, 2022

Updated to liboscal-java 1.0.2 and addressed changes #91

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to add and update SSP Impl Req #94

Ability to add and update SSP Impl Req #94

rgauss commented May 12, 2022

laurelmay left a comment

laurelmay left a comment

hreineck left a comment

hreineck May 14, 2022

rgauss May 17, 2022

hreineck May 14, 2022

rgauss May 17, 2022

hreineck May 14, 2022

hreineck commented May 14, 2022 •

edited

Loading

laurelmay left a comment

rgauss commented May 18, 2022

	throw new OscalObjectConflictException("Implented Requirement already exists");
	throw new OscalObjectConflictException("Implemented Requirement already exists");

	private final OscalObjectMarshaller<ImplementedRequirement> oscalSspImplReqtMarshaller;
	private final OscalObjectMarshaller<ImplementedRequirement> oscalSspImplReqMarshaller;

		@PostMapping(value = "/system-security-plans/{id}/control-implementation/"
		+ "implemented-requirements/{implementedRequirementId}",

Ability to add and update SSP Impl Req #94

Ability to add and update SSP Impl Req #94

Conversation

rgauss commented May 12, 2022

laurelmay left a comment

Choose a reason for hiding this comment

laurelmay left a comment

Choose a reason for hiding this comment

hreineck left a comment

Choose a reason for hiding this comment

hreineck May 14, 2022

Choose a reason for hiding this comment

rgauss May 17, 2022

Choose a reason for hiding this comment

hreineck May 14, 2022

Choose a reason for hiding this comment

rgauss May 17, 2022

Choose a reason for hiding this comment

hreineck May 14, 2022

Choose a reason for hiding this comment

hreineck commented May 14, 2022 • edited Loading

laurelmay left a comment

Choose a reason for hiding this comment

rgauss commented May 18, 2022

hreineck commented May 14, 2022 •

edited

Loading