Arazzo Sampler V1 - #1682
Conversation
| import org.evomaster.core.search.service.mutator.StandardMutator | ||
| import org.evomaster.core.search.service.mutator.StructureMutator | ||
|
|
||
| class ArazzoRestModule(private val bindRemote: Boolean = true) : RestBaseModule() { |
There was a problem hiding this comment.
After a more careful consideration, this class should be deleted.
| import org.evomaster.core.search.service.mutator.MutatedGeneSpecification | ||
| import org.evomaster.core.sql.SqlInsertBuilder | ||
|
|
||
| class ArazzoStructureMutator : ApiWsStructureMutator() { |
There was a problem hiding this comment.
I am not sure if we need a specific mutator for Arazzo? I think we need to discuss this with Andrea
There was a problem hiding this comment.
We can discuss it. However, the MIO algorithm needs to use the mutator. We could use an existing mutator, but that might break the workflow logic.
|
|
||
| import com.google.inject.Inject | ||
| import com.webfuzzing.arazzo.access.ArazzoAccess | ||
| import com.webfuzzing.arazzo.models.domain.Workflow |
There was a problem hiding this comment.
in a future PR, rename Workflow to ArazzoWorkflow
There was a problem hiding this comment.
add as a //TODO to rename it to ArazzoWorkflow
| lateinit var skippedEndpoints : List<Endpoint> | ||
| private set | ||
|
|
||
| var workflowsArazzo = mutableListOf<Workflow>() |
There was a problem hiding this comment.
rename field to arazzoWorkflows
| var workflowsArazzo = mutableListOf<Workflow>() | ||
| private set | ||
|
|
||
| lateinit var workflowsArazzoById: Map<String, Workflow> |
There was a problem hiding this comment.
rename field to to arazzoWorkflowsById
| return listOfNotNull(findActionForOperation(step.operationId)) | ||
| } | ||
| if (!step.workflowId.isNullOrBlank()) { | ||
| val nested = workflowsArazzoById[step.workflowId] ?: return emptyList() |
There was a problem hiding this comment.
but it is expected for the workflowId to be missing? Shouldn't this signal an exception?
There was a problem hiding this comment.
Yes, it is better for it to throw an exception. However, the ideal approach would be for the ArazzoParser to perform the validations and guarantee a valid Arazzo document.
| /** | ||
| * Every Step Arazzo has its corresponding RestCallAction in the actionCluster | ||
| */ | ||
| private fun findActionForOperation(operationId: String): RestCallAction? { |
There was a problem hiding this comment.
can operationId be null? If not, please add a Objects.requireNonNull()
There was a problem hiding this comment.
I am adding an exception.
| val nested = workflowsArazzoById[step.workflowId] ?: return emptyList() | ||
| return nested.steps.flatMap { resolveStep(it) } | ||
| } | ||
| return emptyList() |
There was a problem hiding this comment.
I am not sure about this recursive approach, could you change it to a iterative approach to build the RestCallActions?
There was a problem hiding this comment.
Done. Refactored to an iterative solution using ArrayDeque.
| val template = actionCluster.values | ||
| .filterIsInstance<RestCallAction>() | ||
| .find { it.operationId == operationId } | ||
| return template?.copy() as? RestCallAction |
There was a problem hiding this comment.
what happen if no operationId is found? Is it expected?
There was a problem hiding this comment.
I am adding an exception.
|
|
||
| enum class ArazzoStrategy { | ||
| NONE, | ||
| ENABLED |
There was a problem hiding this comment.
ENABLED is not declarative enough. Please replace with a more declarative name and add a Javadoc to describe this strategy
There was a problem hiding this comment.
How about calling it WORKFLOW?
There was a problem hiding this comment.
@daniellopera15 was it modified to WORKFLOW? i still see ENABLED here
| import javax.annotation.PostConstruct | ||
| import kotlin.sequences.forEach | ||
|
|
||
|
|
There was a problem hiding this comment.
Please revert changes to this file if they are not needed (this seems refactoring such as formatting and unused imports)
|
|
||
| enum class ArazzoStrategy { | ||
| NONE, | ||
| ENABLED |
| <dependency> | ||
| <groupId>org.evomaster</groupId> | ||
| <artifactId>arazzo-parser</artifactId> | ||
| <scope>compile</scope> |
There was a problem hiding this comment.
why the compiler scope? The parser will be required in the distribution to be able to parse Arazzo specs within the evomaster.jar, right?
There was a problem hiding this comment.
The default scope for dependencies is compile. I'm going to remove it.
| EMConfig.ProblemType.REST -> { | ||
| if (config.blackBox) { | ||
| if (config.isEnabledArazzoStrategy()) { | ||
| ArazzoRestModule() |
There was a problem hiding this comment.
I am not sure that an ArazzoRestModule is required. I think Arazzo sampling should be possible to enable in BlackBoxRestModule, ResourceRestModule and RestModule. The Arazzo sampling looks more like a sampling strategy.
There was a problem hiding this comment.
That’s a possibility. I’m going to look into making a commit that includes the Sampler but not the Module. That way, we’d avoid the Mutator issue.
| import org.evomaster.core.search.service.mutator.StandardMutator | ||
| import org.evomaster.core.search.service.mutator.StructureMutator | ||
|
|
||
| class ArazzoRestModule(private val bindRemote: Boolean = true) : RestBaseModule() { |
There was a problem hiding this comment.
After a more careful consideration, this class should be deleted.
|
|
||
| enum class ArazzoStrategy { | ||
| NONE, | ||
| ENABLED |
There was a problem hiding this comment.
@daniellopera15 was it modified to WORKFLOW? i still see ENABLED here
| /** | ||
| * Enum that enables and disables the generation of individuals based on Arazzo Workflows. | ||
| */ | ||
| enum class ArazzoStrategy { |
There was a problem hiding this comment.
if it is only two option on/off, why is this an enum and not a boolean?
There was a problem hiding this comment.
or are there already plans to add more in future? in that case, leave a TODO comment
There was a problem hiding this comment.
No. At the moment, there are no plans to add more in the future. We're going to switch it to a boolean.
|
|
||
| @Experimental | ||
| @Cfg("Probability of controlling the creation of Arazzo individuals.") | ||
| @Probability |
There was a problem hiding this comment.
i guess that should add activating=false, as controlled by arazzoStrategy.
|
|
||
|
|
||
| @Experimental | ||
| @Cfg("arazzo location in disk") |
There was a problem hiding this comment.
Arazzo path location on disk
|
|
||
| @Experimental | ||
| @Cfg("arazzo location in disk") | ||
| var arazzoLocation: String? = null |
There was a problem hiding this comment.
can be ="", with ExistingPath(val canBeBlank: Boolean, val shouldBeWritable: Boolean) having true,false
| */ | ||
| fun load(openAPI: OpenAPI, location: String?) { | ||
| if (location.isNullOrBlank()) { | ||
| throw ConfigProblemException("arazzoLocation must not be null when Arazzo strategy is enabled") |
There was a problem hiding this comment.
Arazzon strategy will be on by default in future, with location empty by default
| } | ||
| val workflows = readArazzoWorkflows(openAPI, location) | ||
| if (workflows.isEmpty()) { | ||
| throw ConfigProblemException("There must be at least one Arazzo workflow.") |
There was a problem hiding this comment.
in this error message, add info on the location
|
|
||
| private fun readArazzoWorkflows(openAPI: OpenAPI, location: String): List<Workflow> { | ||
| val arazzoText = ArazzoAccess.readFromDisk(location) | ||
| return ArazzoParser.parse(arazzoText, openAPI).workflows |
There was a problem hiding this comment.
maybe the parse should be in a try/catch, throwing a ConfigProblemException if any error? eg, what happens if user specify wrong non-Arazzo file by mistake?
| * Choose a random workflow | ||
| */ | ||
| fun sampleAtRandom( | ||
| actionCluster: Map<String, Action>, |
There was a problem hiding this comment.
why is this given as input, and not read directly from an injected field holding ref to the singleton sampler?
There was a problem hiding this comment.
Are you talking about doing something like this?
@Inject
private lateinit var sampler: AbstractRestSampler
There was a problem hiding this comment.
yes. by the time sampleAtRandom is called, the singletons should all be already initialized
| actionCluster: Map<String, Action>, | ||
| createIndividual: (SampleType, MutableList<RestCallAction>) -> RestIndividual, | ||
| ): RestIndividual { | ||
| val actions = buildArazzoRestCallActions(workflow.steps, actionCluster) |
There was a problem hiding this comment.
should clarify here in a comment that, for the time being, we are only considering one single OpenAPI schema fro the workflow, and ignoring cases of arazzo spanning different APIs
No description provided.