With the current fcli output framework, we do a lot of dynamic interface-based lookups, i.e., AbstractOutputCommand checks whether the current command is an instance of IBaseRequestSupplier or IJsonNodeSupplier, from other places we check whether the command or product helper implements IRecordTransformer, IInputTransformer or IActionCommandResultSupplier, and we look up IProductHelper and INextPageUrlProducer instances from various places.
Initially this seemed like a good idea as it allows for simple command implementations, but as we kept adding more features, this became relatively complex and causes some limitations:
It's not always clear where the information provided by a particular interface is being used
We need to navigate through commands and mixins to see which ones implement a particular interface
Most interface methods don't explicitly accept a UnirestInstance, so if such methods need to make additional requests, they need to look up an IUnirestInstanceSupplier
With the new fcli actions framework, we need to integrate with the output framework to collect records produced by a command, rather than just having the command supply those records directly
Current output framework doesn't support interrupting record processing; even if the actions framework doesn't need any more records (for example due to breakIf instruction), the output framework will continue iterating over all records and potentially load additional pages
It's not possible to share data between different interface methods
A good example of the latter is the fcli ssc issue list command. The SSC API only returns folderId and folderGuid, not folder name, making it difficult for users to query issues by folder or otherwise determining what folder an issue is in. Ideally, we'd use a record transformer to add a folderName property, but to do so, we'd need to have the filter set data. With the current architecture, we'd either need to store the filter set descriptor in an instance variable (which is a code smell as command instances may be reused, similar to how you shouldn't use instance variables in Java servlets to store request-specific data), or have the transformRecord() method load the filter set descriptor again and again (for every individual record) on each invocation (which would obviously be very bad for performance).
Essentially, the output and actions frameworks just needs to process a set of records, and shouldn't need to worry about next page URL producers, record transformers, ... So, possibly it would be better to have command implementations return something that simply produces such a set of records, for example represented as a dynamic Java (ordered) Stream (not sure how easy it is to produce such a stream from an HTTP response and attach functionality like loading multiple pages or doing transformations) or more traditionally, have interfaces like the following:
Taking the latter approach as an example, both the output and actions framework would provide one or more IRecordConsumer implementations for either writing or collecting records, returning Break.TRUE if necessary to tell the IRecordProducer to stop providing any further records (for reference, the Stream equivalent for optional breaking would be something like Stream::takeWhile, but we'd need an ordered stream for this to work properly).
Command implementations would implement the IRecordProducerSupplier interface and still (indirectly) extend for example AbstractOutputCommand, which will connect the command-provided IRecordProducer and output framework provided IRecordConsumer to produce command output. The actions framework can simply call getRecordProducer().processRecords(actionRecordCollector).
For convenience, product-specific classes like AbstractSSCOutputCommand could still implement getRecordProducer() by getting the UnirestInstance and then calling the abstract getRecordProducer(UnirestInstance) method to allow command implementations to easily access UnirestInstance, or command implementations could simply get the UnirestInstance from the superclass or explicit SSCUnirestInstanceSupplierMixin field; the latter may be better to avoid the need for multiple superclasses like AbstractSCSastControllerOutputCommand and AbstractSCSastSSCOutputCommand in modules that may access multiple systems.
With all of the above, the IRecordProducer would be responsible for things like paging and input & record transformations (potentially including adding a result column like currently done by IActionCommandResultSupplier). Not sure how we should handle current ISingularSupplier as it doesn't fit any of the interface methods listed above; we could:
Keep the current lookup-based approach on the command instance
Have an additional method isSingular() (and potentially other metadata methods) on either IRecordProducer or maybe IRecordProducerSupplier (but then they wouldn't be single-method functional interfaces anymore, not sure whether that's relevant/useful)
Instead of having commands return an IRecordProducer directly, return some other interface that provides both the getRecordProducer() and isSingular() methods (to allow IRecordProducer to still be a functional interface)
Of course, we need to provide some infrastructure for easily building IRecordProducer instances, both for handling generic aspects like record transformations, and product-specific aspects like paging, for example to allow command implementations to do something like the below:
public IRecordProducer getRecordProducer() {
var unirest = sscUnirestMixin.getUnirestInstance();
var appVersionDescriptor = parentResolver.getAppVersion(unirest);
return SSCRecordProducer.builder()
.unirest(unirest) // or: .cmd(this), to allow SSCRecordProducer to invoke cmd.getUnirestInstanceSupplier().getUnirestInstance()
.baseRequest(unirest.get(...)) // or: .jsonNode(SomeHelper.getXyzDescriptor().asJsonNode())
.recordTransformer(record->this.transformRecord(unirest, appVersionDescriptor, record)
.build();
}
This sample code should mostly work, but only because we never close any UnirestInstance until fcli termination (through GenericUnirestFactory.shutdown(), which is (maybe) something that we'd like to get rid of:
Current approach avoids recreating similar UnirestInstance instances multiple times, and allows for easily accessing a UnirestInstance from anywhere. For example in the code above, an open UnirestInstance is needed by the getRecordProducer() method (to load app version descriptor), but also by both the processRecords() method (to execute the GET request and perform paging) and (in this example) the transformRecord() method. If we'd use a try-with-resources block to close the UnirestInstance when this method returns, the processRecords() and transformRecord() methods would be accessing a UnirestInstance that's already been closed. Alternatively they could instead invoke IUnirestInstanceSupplier.getUnirestInstance() to retrieve a UnirestInstance, but this would potentially mean that instances are being recreated for every record being processed, when the transformRecord method is being called.
Current long-lives 'static' UnirestInstance instances could cause issues and unexpected behavior in some cases. For example, GenericUnirestFactory.getUnirestInstance() takes a configurer, but the configurer is only invoked if theUnirestInstancefor the given key wasn't already configured before. This basically means that once configured, the configuration cannot be easily changed. For example, if any fcli action runs anfcli config proxy add/update` command, this configuration potentially won't take effect until after fcli is being restarted. This would be an even bigger issue if we ever introduce 'fcli shell' capabilities.
If (because of the second point) we ever change they way we manage UnirestInstance instances, we need to invent some way to create that instance when command execution starts and close it once command execution finishes, and make that instance (easily) available everywhere where it may be needed, like all of the methods mentioned above. Some potential ideas (that require further research):
Have command implementations optionally provide a close() method that closes the UnirestInstance (and any other resources) managed by the unirest mixin
Have some generic 'context' object that's created by FortifyCommandExecutor and accessible to command implementations, where any type of data can be stored, including UnirestInstance, with the context and any closeable objects within it getting closed after the command has completed. Potentially, this could also be used to share data between different methods of current command implementations, like having getJsonNode() store the current application version descriptor, which can then be accessed by the transformRecord() method.
With the current fcli output framework, we do a lot of dynamic interface-based lookups, i.e.,
AbstractOutputCommandchecks whether the current command is an instance ofIBaseRequestSupplierorIJsonNodeSupplier, from other places we check whether the command or product helper implementsIRecordTransformer,IInputTransformerorIActionCommandResultSupplier, and we look upIProductHelperandINextPageUrlProducerinstances from various places.Initially this seemed like a good idea as it allows for simple command implementations, but as we kept adding more features, this became relatively complex and causes some limitations:
UnirestInstance, so if such methods need to make additional requests, they need to look up anIUnirestInstanceSupplierbreakIfinstruction), the output framework will continue iterating over all records and potentially load additional pagesA good example of the latter is the
fcli ssc issue listcommand. The SSC API only returnsfolderIdandfolderGuid, not folder name, making it difficult for users to query issues by folder or otherwise determining what folder an issue is in. Ideally, we'd use a record transformer to add afolderNameproperty, but to do so, we'd need to have the filter set data. With the current architecture, we'd either need to store the filter set descriptor in an instance variable (which is a code smell as command instances may be reused, similar to how you shouldn't use instance variables in Java servlets to store request-specific data), or have thetransformRecord()method load the filter set descriptor again and again (for every individual record) on each invocation (which would obviously be very bad for performance).Essentially, the output and actions frameworks just needs to process a set of records, and shouldn't need to worry about next page URL producers, record transformers, ... So, possibly it would be better to have command implementations return something that simply produces such a set of records, for example represented as a dynamic Java (ordered)
Stream(not sure how easy it is to produce such a stream from an HTTP response and attach functionality like loading multiple pages or doing transformations) or more traditionally, have interfaces like the following:IRecordProducerSupplier{ IRecordProducer getRecordProducer() }`IRecordProducer { void processRecords(IRecordConsumer consumer) }IRecordConsumer { Break consume(ObjectNode record)Taking the latter approach as an example, both the output and actions framework would provide one or more
IRecordConsumerimplementations for either writing or collecting records, returningBreak.TRUEif necessary to tell theIRecordProducerto stop providing any further records (for reference, theStreamequivalent for optional breaking would be something likeStream::takeWhile, but we'd need an ordered stream for this to work properly).Command implementations would implement the
IRecordProducerSupplierinterface and still (indirectly) extend for exampleAbstractOutputCommand, which will connect the command-providedIRecordProducerand output framework providedIRecordConsumerto produce command output. The actions framework can simply callgetRecordProducer().processRecords(actionRecordCollector).For convenience, product-specific classes like
AbstractSSCOutputCommandcould still implementgetRecordProducer()by getting theUnirestInstanceand then calling the abstractgetRecordProducer(UnirestInstance)method to allow command implementations to easily accessUnirestInstance, or command implementations could simply get theUnirestInstancefrom the superclass or explicitSSCUnirestInstanceSupplierMixinfield; the latter may be better to avoid the need for multiple superclasses likeAbstractSCSastControllerOutputCommandandAbstractSCSastSSCOutputCommandin modules that may access multiple systems.With all of the above, the
IRecordProducerwould be responsible for things like paging and input & record transformations (potentially including adding a result column like currently done byIActionCommandResultSupplier). Not sure how we should handle currentISingularSupplieras it doesn't fit any of the interface methods listed above; we could:isSingular()(and potentially other metadata methods) on eitherIRecordProduceror maybeIRecordProducerSupplier(but then they wouldn't be single-method functional interfaces anymore, not sure whether that's relevant/useful)IRecordProducerdirectly, return some other interface that provides both thegetRecordProducer()andisSingular()methods (to allowIRecordProducerto still be a functional interface)Of course, we need to provide some infrastructure for easily building
IRecordProducerinstances, both for handling generic aspects like record transformations, and product-specific aspects like paging, for example to allow command implementations to do something like the below:This sample code should mostly work, but only because we never close any
UnirestInstanceuntil fcli termination (throughGenericUnirestFactory.shutdown(), which is (maybe) something that we'd like to get rid of:UnirestInstanceinstances multiple times, and allows for easily accessing aUnirestInstancefrom anywhere. For example in the code above, an openUnirestInstanceis needed by thegetRecordProducer()method (to load app version descriptor), but also by both theprocessRecords()method (to execute the GET request and perform paging) and (in this example) thetransformRecord()method. If we'd use a try-with-resources block to close theUnirestInstancewhen this method returns, theprocessRecords()andtransformRecord()methods would be accessing aUnirestInstancethat's already been closed. Alternatively they could instead invokeIUnirestInstanceSupplier.getUnirestInstance()to retrieve aUnirestInstance, but this would potentially mean that instances are being recreated for every record being processed, when thetransformRecordmethod is being called.UnirestInstanceinstances could cause issues and unexpected behavior in some cases. For example,GenericUnirestFactory.getUnirestInstance() takes a configurer, but the configurer is only invoked if theUnirestInstancefor the given key wasn't already configured before. This basically means that once configured, the configuration cannot be easily changed. For example, if any fcli action runs anfcli config proxy add/update` command, this configuration potentially won't take effect until after fcli is being restarted. This would be an even bigger issue if we ever introduce 'fcli shell' capabilities.If (because of the second point) we ever change they way we manage
UnirestInstanceinstances, we need to invent some way to create that instance when command execution starts and close it once command execution finishes, and make that instance (easily) available everywhere where it may be needed, like all of the methods mentioned above. Some potential ideas (that require further research):close()method that closes theUnirestInstance(and any other resources) managed by the unirest mixinFortifyCommandExecutorand accessible to command implementations, where any type of data can be stored, includingUnirestInstance, with the context and any closeable objects within it getting closed after the command has completed. Potentially, this could also be used to share data between different methods of current command implementations, like havinggetJsonNode()store the current application version descriptor, which can then be accessed by thetransformRecord()method.