Cucumber-jvm: DataTable - Automatic conversion to custom type stopped working

[kathyrollo]

Summary

After updating cucumber-testng (and subsequently all transient dependencies) from 2.4.0 to 3.0.2 in pom.xml, automatic conversion for datatables using custom types stopped working and now throws UndefinedDataTableTypeException.

Expected Behavior

Only change required is to update imports to import io.cucumber.datatable.DataTable; and tests to run as per usual.

Current Behavior

Getting this exception from steps that worked prior to update:

io.cucumber.datatable.UndefinedDataTableTypeException: Can't convert DataTable to List. Please register a DataTableType with a TableEntryTransformer or TableRowTransformer for class jcucumberng.steps.pojos.Income

~~~
// Feature
When I Enter My Regular Income Sources
| name | amount | frequency |
| Salary | 25000 | every 2 weeks |

// Stepdef
@When("^I Enter My Regular Income Sources$")
public void I_Enter_My_Regular_Income_Sources(DataTable dataTable) throws Throwable {
List incomes = dataTable.asList(Income.class);

// More code

}

// Custom type
public class Income {

private String name = null;
private String amount = null;
private String frequency = null;

public Income(String name, String amount, String frequency) {
    this.name = name;
    this.amount = amount;
    this.frequency = frequency;
}

// Getters and setters

}
~~~

Possible Solution

Show more useful comments/javadocs for DataTable specially if updating from v2.x.x to v3.x.x for major changes to its usage. Allow easier automatic conversion like before.

Context & Motivation

A revamp of the DataTable usage will break a lot of existing tests and can totally prevent people from updating to v3.x.x. It also outdates a lot of widely available tutorials and references.

[mpkorstanje]

I am going to assume you've found what you've needed. If not, or if anybody else searches github for this specific problem:

The release announcement for cucumber-jvm 3.0.0 includes some basic instructions on how to use the datatables. It also contains a brief explanation on removal of xstream which underpinned the automatic conversions.

[aslakhellesoy]

I’ve got a pending PR for this (haven’t pushed yet). I’m reopening since I plan to fix it next week.

Context: I just ran a public training where a few people remarked that while the new data tables are great, they do require more boilerplate/configuration code. I think I’ve found a pragmatic middle ground, where you’ll still need to register a DataTableType, but doing so will be a one-liner.

[mpkorstanje]

Excellent. It was one of those problems I hoped some one else would solve.

[kathyrollo]

@mpkorstanje Thank you for linking the release announcement. Yes, I also asked for help at SO where I posted my solution for anyone who may be looking.

@aslakhellesoy One-liner sounds like a good compromise. I also run Cucumber-JVM training sessions to colleagues at work (many with little to no programming experience) who want to get into test automation. I did realise the additional Configurer class adds to the learning curve and considered if sticking to v2.4.0 is better for the time being.

A comprehensive and easily searchable migration guide/documentation from v2.x.x to v3.x.x is quite timely. Thank you for your work.

[mlvandijk]

@kathyrollo I've been planning a migration guide, but haven't gotten around to it. Any chance you'd be willing to help? If so, please have a look at the documentation project and/or join us on Slack to discuss (you can get access here)

[grasshopper7]

@mlvandijk Have been attempting to document migration here - https://github.com/grasshopper7/cuke3-migration. Its a work in progress and kind of covered ParameterType till now. Hopefully finish up DataTableType soon. Was hoping to create an article out of it and post it online. If you like the end product, happy to merge it into existing documentation.

[mlvandijk]

@grasshopper7 Contributions to documentation would be very much appreciated!

[kathyrollo]

@mlvandijk Thank you, @grasshopper7 has a lovely writeup in progress. I'll look into the docu repo as well.

[mlvandijk]

@kathyrollo We just added some info on Type Registry to the docs; you can find it in Cucumber configuration. (@grasshopper7 Please feel free to add to it, if anything is still missing)

[mlvandijk]

And ofcourse, you too @kathyrollo - feel free to add clarification yourself, or let us know what could be improved ;)

[mlvandijk]

Some information on SO

[kathyrollo]

@mlvandijk That SO thread is me. :)

[lsuski]

I think that oneliner introduced in https://github.com/cucumber/cucumber/pull/408 PR is still too much, I have hundreds of data table classes in couple of projects and need to register all of them. I think that this oneliner should be default fallback and no one would need to do anything while migrating to cucumber v3.

[aslakhellesoy]

@lsuski if this is important to you we’d welcome a pull request.

[lsuski]

Yeah, I've just started working on this

[lsuski]

As usual the problem is more complicated. There are 2 things that does not work in v3:

1. Automatic object creation for class without default empty constructor (Gson can do this - probably with Unsafe, without additional annotations)
2. Automatic conversion of multiword space-separated column names to Java field name, e.g. "column no 1" to "columnNo1".

Therefore I think that the best solution would be to slightly change DataTableTypeRegistry to allow registering types taking into account inheritance.

This code could be sufficient to migrate to v3:

override fun configureTypeRegistry(typeRegistry: TypeRegistry) {
        typeRegistry.defineDataTableType(DataTableType(Object::class.java, TableEntryTransformer {map,type->
            val map = map.mapKeys { CamelCaseConcatenator().concatenate(it.key.splitBySpace().toTypedArray()) }
            val objectMapper = Gson()
            objectMapper.fromJson(JsonObject().apply { map.forEach { addProperty(it.key,it.value) } }, type)
        }))
    }

To have this work:

1. DataTableTypeRegistry.lookupTableTypeByType(Type) should search for best matching type
2. TableEntryTransformer.transform(Map) should have additional parameter with actual type declared in step method.

Do you think this is a good idea, so I could make a PR?

[aslakhellesoy]

sun.misc.Unsafe doesn't work in Java 9 unless you use hacks, so we shouldn't rely on that.

The code you've pasted looks like Kotlin. I assume that was just for illustration purposes - I don't assume you are suggesting we use Kotlin?

Regarding 1 - what criteria would you use to find a matching type?
Regarding 2 - sounds like a good idea to me.

What are your thoughts, @mpkorstanje / @mlvandijk?

[aslakhellesoy]

Disregard my Kotlin comment - it's obviously user code now that I read it again :-)

[lsuski]

1. I use such strategy in my apps
   
   
   
   • find exact type
   
   
   • if not found find all supertypes in registry
   
   
   • sort them by inheritance level and get closest
     
     superclass
   
   

Regarding Unsafe its up to user, on Android it works, on another platform one can use Jackson or something else. That's why I've abandoned the idea of putting default fallback to Cucumber

[mpkorstanje]

TableEntryTransformer.transform(Map) should have additional parameter with actual type declared in step method.

This converter should be given a special status. There can only be one.

It will match any list(X), where X is not a collection and there is no other converter for list(X). This requires some extra logic in the lookup.

I don't see a problem here, but perhaps we can rename it to default object mapper? Also make it work for the cell converter.

DataTableTypeRegistry.lookupTableTypeByType(Type) should search for best matching type

This sounds like something you can implement in your implementation of the default object mapper.

I don't think this is generally applicable.

[mpkorstanje]

Automatic conversion of multiword space-separated column names to Java field name, e.g. "column no 1" to "columnNo1".

This too can go into your default implementation of the object mapper. I know Jackson allows plugins that do this.

[lsuski]

Default mapper/transformer sounds good for me as well.

[mpkorstanje]

Sure. Transformer is probably better given the context. But we can bikeshed about that later.

[aslakhellesoy]

👍

[lsuski]

Couple of problems:

1. Do you preffer to add second parameter to Table...Transformer interfaces - this will break compatibility and all the people who already migrated would have to change their code - or we should add other interfaces
2. DataTableTypeRegistry returns DataTableType by type, not knowing if this is cell, row or table type as knowledge about that belongs to DataTableTypeRegistryTableConverter - as a user I'd like to register default transformer for row/entry and this complicates it a lot - in most simple case I just need to register DataTableType for List<T> where T is not known by me and get specific T type in transformer transform(Map<String, String>, Type) and pass it to ObjectMapper or Gson - currently I don't have any idea how to implement it without hard refactoring to achieve 2 goals: simplicity of cucumber implementation and simplicity of use

[mpkorstanje]

I reckon the configuration might look like this:

public class TypeRegistryConfiguration implements TypeRegistryConfigurer {

    @Override
    public Locale locale() {
        return ENGLISH;
    }

    @Override
    public void configureTypeRegistry(TypeRegistry typeRegistry) {
        typeRegistry.defaultDataTableTransformer(new DefaultDataTableTransformer() {
            ObjectMapper objectMapper = new ObjectMapper();

            public <T> T transform(Map<String, String> value, Class<T> type) {
                return objectMapper.convertValue(value, type);
            }

            @Override
            public <T> T transform(String value, Class<T> type) {
                return objectMapper.convertValue(value, type);
            }
        });
    }
}

Then look up might use it like this:

            public DataTableType lookupTableTypeByType(final Type tableType) {
                JavaType targetType = constructType(tableType);
                DataTableType dataTableType = tableTypeByType.get(targetType);

                if(dataTableType != null){
                    return dataTableType;
                }

                if(defaultDataTableTransformer == null){
                    return null;
                }
                if(!targetType.isCollectionLikeType()){
                    return  null;
                }

                JavaType contentType = targetType.getContentType();
                if(contentType.isCollectionLikeType()){
                    return new DataTableType(contentType.getContentType().getRawClass(), targetType, new DataTableType.TableCellTransformer<>(new TableCellTransformer<Object>() {
                        //Use DefaultDataTableTransformer.transform(String value, Class<T> type) here
                    }));

                } else {
                    return new DataTableType(contentType.getRawClass(), targetType, new DataTableType.TableEntryTransformer<>(new TableEntryTransformer<Object>() {
                        //Use DefaultDataTableTransformer.transform(Map<String, String> value, Class<T> type) here
                    }));
                }
            }

The trick is that rather then storing the transformer, it creates one adhoc for the tableType. By looking at the structure of the type it it can determine if this should be a table entry or table cell transformer.

edit: Once this works, I reckon the next step would be to look for a way to keep everything after if(defaultDataTableTransformer == null) contained in DataTableType so we can do defaultDataTableTransformer.for(tableType).

[lsuski]

I think that similar logic that you proposed is in DataTableTypeRegistryTableConverter.transform so I wouldn't duplicate it in lookupTableTypeByType. Maybe DataTableTypeRegistry should only return DefaultDataTableTransformer and DataTableTypeRegistryTableConverter should decide when to call proper method

[mpkorstanje]

That would work too I think. Might be neater even.

[mpkorstanje]

@lsuski branch has been merged. If you are using 4.0.0-SNAPSHOT you can now register default transformers for table cells and table entries: For example:

private class JacksonDataTableTransformer implements TableEntryByTypeTransformer, TableCellByTypeTransformer {

    ObjectMapper objectMapper = new com.fasterxml.jackson.databind.ObjectMapper();

    @Override
    public <T> T transform(String value, Class<T> cellType) {
        return objectMapper.convertValue(value, cellType);
    }

    @Override
    public <T> T transform(Map<String, String> entry, Class<T> type, TableCellByTypeTransformer cellTransformer) {
        return objectMapper.convertValue(entry, type);
    }
}

I am refraining from actually setting a default mapper just yet.

[mpkorstanje]

Setting a default mapper (jackson in our case) requires that header values are written in a specific way and must be derived from the property name (not the other way around).

So if our property is: dataTableExampleMapper what should the table header be?

A few suggestions:

Data Table Example Mapper | data table example mapper | dataTableExampleMapper | DataTableExampleMapper |

[mpkorstanje]

Alternatively we can also rename the table headers before doing the mapping. This is a bit more flexible.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

[stale[bot]]

This issue has been automatically closed because of inactivity. You can support the Cucumber core team on opencollective.

[mpkorstanje]

Still not solved.

[Madhusudan-Devaraju]

I'm facing the problem as well. Below is the error found in the stack trace.

cucumber-Datatable-error.txt

This is now resolved for me after updating to the newer version of cucumber-core.

io.cucumber
cucumber-core
4.2.6

And by converting the DataTable object to List > type i.e..,
public void form_is_open_and_user_enters_the_below_data(DataTable data) {
List > userInfo = data.asLists();
}

[mpkorstanje]

Setting a default mapper (jackson in our case) requires that header values are written in a specific way and must be derived from the property name (not the other way around).

So if our property is: dataTableExampleMapper what should the table header be?

A few suggestions:

Data Table Example Mapper | data table example mapper | dataTableExampleMapper | DataTableExampleMapper |

I'm going to leave this to the discretion of the people who configure Jackson. The whole point of using an external object mapper is that Cucumber can of load these concerns.