New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RenameProperty causes adding extra columns to fail #461

Closed
koscejev opened this Issue Nov 1, 2017 · 8 comments

Comments

Projects
None yet
2 participants
@koscejev

koscejev commented Nov 1, 2017

Considering MyPojo has two fields ts and action, the following fails on the last line:

    CsvWriter.CsvWriterDSL<MyPojo> csvWriterConfig = CsvWriter.from(MyPojo.class)
      .columns()
      .column("ts", new RenameProperty("timestamp")) // OK
      .column("action"); // fails on this line

The exception:

org.simpleflatmapper.map.MapperBuildingException: Could not find eligible property for 'timestamp' on  class my.pojo.MyPojo not found  See https://github.com/arnaudroger/SimpleFlatMapper/wiki/Errors_PROPERTY_NOT_FOUND

	at org.simpleflatmapper.map.error.RethrowMapperBuilderErrorHandler.propertyNotFound(RethrowMapperBuilderErrorHandler.java:26)
	at org.simpleflatmapper.map.mapper.PropertyMappingsBuilder$1.accept(PropertyMappingsBuilder.java:65)
	at org.simpleflatmapper.map.mapper.PropertyMappingsBuilder$1.accept(PropertyMappingsBuilder.java:62)
	at org.simpleflatmapper.map.mapper.PropertyMappingsBuilder.handleSelfPropertyMetaInvalidation(PropertyMappingsBuilder.java:138)
	at org.simpleflatmapper.map.mapper.PropertyMappingsBuilder._addProperty(PropertyMappingsBuilder.java:106)
	at org.simpleflatmapper.map.mapper.PropertyMappingsBuilder.addProperty(PropertyMappingsBuilder.java:73)
	at org.simpleflatmapper.map.mapper.AbstractConstantTargetMapperBuilder.addColumn(AbstractConstantTargetMapperBuilder.java:85)
	at org.simpleflatmapper.map.mapper.AbstractConstantTargetMapperBuilder.addColumn(AbstractConstantTargetMapperBuilder.java:72)
	at org.simpleflatmapper.csv.CsvWriter$CsvWriterDSL.newColumnMapDSL(CsvWriter.java:400)
	at org.simpleflatmapper.csv.CsvWriter$CsvWriterDSL.column(CsvWriter.java:309)
@arnaudroger

This comment has been minimized.

Show comment
Hide comment
@arnaudroger

arnaudroger Nov 13, 2017

Owner

just seen that sorry for not coming back on it earlier

Owner

arnaudroger commented Nov 13, 2017

just seen that sorry for not coming back on it earlier

@arnaudroger

This comment has been minimized.

Show comment
Hide comment
@arnaudroger

arnaudroger Nov 13, 2017

Owner

what is the structure of MyPojo?
I don't know if I ever check rename property work for writing will add a test and check

Owner

arnaudroger commented Nov 13, 2017

what is the structure of MyPojo?
I don't know if I ever check rename property work for writing will add a test and check

@arnaudroger arnaudroger added this to the 3.14 milestone Nov 13, 2017

@koscejev

This comment has been minimized.

Show comment
Hide comment
@koscejev

koscejev Nov 13, 2017

I tried this with different structures, it doesn't seem to be related to the actual fields.

It seems that rename property is applied initially one way, but when it needs to be "reapplied" (because columns are copied when a new one is added), it's not done fully and the fields are checked against original names, not against renamed ones.

That's just my understanding so far, couldn't investigate in detail. In the end I only did a PoC and had to switch to a different project because of this.

koscejev commented Nov 13, 2017

I tried this with different structures, it doesn't seem to be related to the actual fields.

It seems that rename property is applied initially one way, but when it needs to be "reapplied" (because columns are copied when a new one is added), it's not done fully and the fields are checked against original names, not against renamed ones.

That's just my understanding so far, couldn't investigate in detail. In the end I only did a PoC and had to switch to a different project because of this.

@arnaudroger

This comment has been minimized.

Show comment
Hide comment
@arnaudroger

arnaudroger Nov 14, 2017

Owner

I'm just trying to understand what you where trying to do
is ts in the pojo and you want the column name to be timestamp?

Owner

arnaudroger commented Nov 14, 2017

I'm just trying to understand what you where trying to do
is ts in the pojo and you want the column name to be timestamp?

@arnaudroger

This comment has been minimized.

Show comment
Hide comment
@arnaudroger

arnaudroger Nov 14, 2017

Owner

so just tried by assuming the prop in the object would be ts.

    public static class Pojo461 {
        public final long ts;
        public final String action;
    }

the rename property in that context will actually tell it what is the name in the object.
so here you are saying that "ts" is the output columnname but it actually map to "timestamp"

timestamp does not exists so it does some speculative property mapping and link the column to the actual pojo.toString object.
but when you add another column it backtrack has obiviously that was the wrong choice and throw the exception on timestamp at that time.

RenameProperty was introduce initially on the reading side and the semantic is not really clear there it should actually contains the actual property name.

so if you write

        CsvWriter.CsvWriterDSL<Pojo461> csvWriterConfig = CsvWriter.from(Pojo461.class)
                .columns()
                .column("timestamp", new RenameProperty("ts")) // OK
                .column("action"); // fails on this line
        
        StringBuilder sb = new StringBuilder();
        csvWriterConfig.to(sb).append(new Pojo461(1, "add"));

        System.out.println("sb = " + sb);

it works and prints

timestamp,action
1,add
Owner

arnaudroger commented Nov 14, 2017

so just tried by assuming the prop in the object would be ts.

    public static class Pojo461 {
        public final long ts;
        public final String action;
    }

the rename property in that context will actually tell it what is the name in the object.
so here you are saying that "ts" is the output columnname but it actually map to "timestamp"

timestamp does not exists so it does some speculative property mapping and link the column to the actual pojo.toString object.
but when you add another column it backtrack has obiviously that was the wrong choice and throw the exception on timestamp at that time.

RenameProperty was introduce initially on the reading side and the semantic is not really clear there it should actually contains the actual property name.

so if you write

        CsvWriter.CsvWriterDSL<Pojo461> csvWriterConfig = CsvWriter.from(Pojo461.class)
                .columns()
                .column("timestamp", new RenameProperty("ts")) // OK
                .column("action"); // fails on this line
        
        StringBuilder sb = new StringBuilder();
        csvWriterConfig.to(sb).append(new Pojo461(1, "add"));

        System.out.println("sb = " + sb);

it works and prints

timestamp,action
1,add
@arnaudroger

This comment has been minimized.

Show comment
Hide comment
@arnaudroger

arnaudroger Nov 14, 2017

Owner

I'm adding some javadoc on the RenameProperty to make that clearer.
and will add some doc too.

Owner

arnaudroger commented Nov 14, 2017

I'm adding some javadoc on the RenameProperty to make that clearer.
and will add some doc too.

@koscejev

This comment has been minimized.

Show comment
Hide comment
@koscejev

koscejev Nov 14, 2017

Indeed, looking at your example, I was applying it in the opposite direction. I had property ts and wanted to write it out as timestamp. The new javadoc makes it more clear, nice!

However, in that case I'm surprised it failed on the next column, not the ts column, when I used what is basically an invalid property name. May I suggest also a negative test case that should result in immediate failure when RenameProperty references a non-existing bean property?

koscejev commented Nov 14, 2017

Indeed, looking at your example, I was applying it in the opposite direction. I had property ts and wanted to write it out as timestamp. The new javadoc makes it more clear, nice!

However, in that case I'm surprised it failed on the next column, not the ts column, when I used what is basically an invalid property name. May I suggest also a negative test case that should result in immediate failure when RenameProperty references a non-existing bean property?

@arnaudroger arnaudroger reopened this Nov 14, 2017

@arnaudroger

This comment has been minimized.

Show comment
Hide comment
@arnaudroger

arnaudroger Nov 14, 2017

Owner

not a bad idea, no point trying to speculate when the RenameProperty should be an exact match

Owner

arnaudroger commented Nov 14, 2017

not a bad idea, no point trying to speculate when the RenameProperty should be an exact match

@arnaudroger arnaudroger modified the milestone: 3.14.0 Nov 14, 2017

@arnaudroger arnaudroger added this to the 4.0.0 milestone Jun 13, 2018

arnaudroger added a commit that referenced this issue Jun 14, 2018

arnaudroger added a commit that referenced this issue Jun 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment