cucumber / cucumber-jvm

Cucumber for the JVM
https://cucumber.io
MIT License
2.71k stars 2.03k forks source link

Support multiple DocStringTypes with the same contentType #2343

Closed PostelnicuGeorge closed 2 years ago

PostelnicuGeorge commented 3 years ago

As far as documentation goes, the fact that jackson json library is used and DataTableType can have multiple return types, it's not that obvious why DocStringType has only 1 return type for all feature files/project?

Given the following example:
  Scenario: Test whether DocStringType allows conversion of 2 different types of records
    Given I have the following cars
    """json
    [
    {
    "name": "Murcielago",
    "brand": "Lamborghini"
    },
    {
    "name": "Sandero",
    "brand": "Dacia"
    }
    ]
    """
    And one animal I like
    """json
    {
    "name": "Lion",
    "domesticated": false
    }
    """
public class DemoStepDef {

    @DocStringType(contentType = "json")
    public List<Car> convertCars(String json) throws JsonProcessingException {
        return new ObjectMapper().readValue(json,
                new TypeReference<List<Car>>() {});
    }

    @DocStringType(contentType = "json")
    public Animal convertAnimal(String json) throws JsonProcessingException {
        return new ObjectMapper().readValue(json,
                new TypeReference<Animal>() {});
    }

    @Given("I have the following cars")
    public void i_have_cars(List<Car> cars) {
        System.out.println(cars);
    }

    @And("one animal I like")
    public void one_animal_I_like(Animal animal) {
        System.out.println(animal);
    }
}

public class Animal {
    private String name;
    private boolean domesticated;
// setters and getters omitted 
}

public class Car {
    private String name;
    private String brand;
// setters and getters omitted
}

Gives the following error:

**Caused by: io.cucumber.docstring.CucumberDocStringException: There is already docstring type registered for 'json' and com.lumeon.cpm.models.records.Animal.**

My feeling is that if jackson library is allowed for DocStringType annotation, then we should be able to leverage the full potential of it, and not be forced to have JsonNode as the only option. Having extra intermediary methods to convert to the actual types needed for the step definitions.

Work-around at the moment:

public class DemoThatWorksStepDef {

    @DocStringType(contentType = "json")
    public JsonNode json(String json) throws JsonProcessingException {
        return new ObjectMapper().readTree(json);
    }

    public List<Car> convertCars(JsonNode jsonNode) throws IOException {
        return new ObjectMapper().readValue(jsonNode.traverse(),
                new TypeReference<List<Car>>() {});
    }

    public Animal convertAnimal(JsonNode jsonNode) throws IOException {
        return new ObjectMapper().readValue(jsonNode.traverse(),
                new TypeReference<Animal>() {});
    }

    @Given("I have the following cars")
    public void i_have_cars(JsonNode jsonNode) throws IOException {
        System.out.println(convertCars(jsonNode));
    }

    @And("one animal I like")
    public void one_animal_I_like(JsonNode jsonNode) throws IOException {
        System.out.println(convertAnimal(jsonNode));
    }
}
mpkorstanje commented 3 years ago

Ah. Indeed.

Currently the DocStringTypeRegistry maintains a Map<String, DocStringType> and Map<Type, DocStringType> which forces there to be a single doc string type per content type string and java type. This could be a Map<String, Map<Type, DocStringType>> which would accommodate multiple converters for json documents to different java types.

Some care must be when dealing with the empty content type as then the following would be ambiguous:

@DocStringType(contentType = "json")
public List<Car> convertCars(String json) {
}
@DocStringType(contentType = "xml")
public List<Car> convertCars(String xml) {
}

Would you be interested in making a pull request for this?

PostelnicuGeorge commented 3 years ago

@mpkorstanje thanks for the super fast response. I will give it a go, lets see if I can manage :)

ntthaibk commented 11 months ago

hi @mpkorstanje, I'm just stumble upon this and find out we can indeed parse docstringtype into different object type, not just JsonNode, somehow, in our docs on typeregistry doesn't mention it, https://cucumber.io/docs/cucumber/configuration/?lang=java, do you think it's ok to add some documentation on it. I can try to update it if you are ok with it

Thanks and have a nice day

mpkorstanje commented 11 months ago

Sure! I'd be happy to look at a proposal.

Perhaps you could start with this page https://github.com/cucumber/cucumber-jvm/tree/main/cucumber-java#docstring-type and then reference that from the docs site. It's a bit easier to keep the docs up to date if they're closer to the source code.