Skip to content
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.

Sign up for GitHub

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

Jump to bottom

InfluxDBResultMapper doesnt call POJO setter #670

Open
sjoshid opened this issue Apr 7, 2020 · 6 comments · May be fixed by #676
Open

InfluxDBResultMapper doesnt call POJO setter #670

sjoshid opened this issue Apr 7, 2020 · 6 comments · May be fixed by #676
Assignees
@fmachado
Labels
kind/enhancement question

Comments

@sjoshid
Copy link

sjoshid commented Apr 7, 2020

It is puzzling that InfluxDBResultMapper doesnt call POJO setter.
My POJO looks like

class MyPOJO
{
    @Column(name = "time")
    private Instant time;

    //getter ignored
    public void setTime(Instant time) {  
        this.time = time.truncatedTo(ChronoUnit.SECONDS);
    }
}

Then I do something like

    final List<MyPOJO> resultEvents =  resultMapper.toPOJO(result, MyPOJO.class);

Assumption is setTime would be called for each point fetched. But it doesnt. It somehow bypasses it. time is private. Reflection in play here?
Expectation is that if I remove my setter, toPOJO call would fail. But it doesnt.

I have tried 2.17, 2.7 and 2.9 versions.
@majst01
Copy link
Collaborator

majst01 commented Apr 7, 2020

Hi,
please describe what problem are you facing, the questions above are more regarding implementation details of influxdb-java which can be seen in the source code obviously.

@sjoshid
Copy link
Author

sjoshid commented Apr 7, 2020

@majst01
Im trying to truncate (to seconds) the Instant I received from Influx. If setTime isnt called, I cant do this. I can iterate over the result of toPOJO but that will lead to other....nasty changes on my side.
Im trying to avoid those.

@fmachado
Copy link
Contributor

fmachado commented Apr 7, 2020

@sjoshid yes, you are right: setters and field visibility (default, private, protected or public) are ignored. The reason why I decided to ignore accessory methods were (1) simplicity and (2) the idea was to have a real mapping of the data stored.

Particularly I think it's a bad practice to pass a parameter to a setter (e.g. "foobar") and have it changed to something else (e.g. "foo") but I understand this is allowed by some ORMs and NoSQL drivers and used in some cases.

InfluxDB supports precisions different than ns like m (minutes) or s seconds. Can you store your points with one of these?

You are also welcome to contribute add this feature to the project. Create a PR and we'll review it. :)

@fmachado fmachado self-assigned this Apr 7, 2020
@sjoshid
Copy link
Author

sjoshid commented Apr 7, 2020

@fmachado
I agree it is a bad practice. And so is not following field visibility policies. No?
Keeping aside my reasons to do this, we should follow getter/setter and leave the rest to end users. It's up to them to follow good or bad policies.
I'll try to work on this and open a PR.
Thank you.

@fmachado
Copy link
Contributor

I'm closing this ticket. Feel free to create your PR and we'll reopen this one.

sjoshid pushed a commit to sjoshid/influxdb-java that referenced this issue May 9, 2020
PR for influxdata#670
7477553 
1) Calling field setters if available before falling back to setting fields directly.
sjoshid pushed a commit to sjoshid/influxdb-java that referenced this issue May 9, 2020
PR for influxdata#670
1d1bdea 
1) Calling field setters if available before falling back to setting fields directly.
@sjoshid sjoshid linked a pull request May 9, 2020 that will close this issue
Open
@fmachado fmachado reopened this May 10, 2020
sjoshid pushed a commit to sjoshid/influxdb-java that referenced this issue May 10, 2020
PR for influxdata#670
0db6460 
1) Handling NPE
sjoshid pushed a commit to sjoshid/influxdb-java that referenced this issue May 10, 2020
PR for influxdata#670
5c9bd27 
1) Fixing Checkstyle errors.
@John9570
Copy link

I'd also like this functionality, whats the status on the review for this? This would be a super useful feature

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

@fmachado fmachado

Labels
kind/enhancement question
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixes issue: InfluxDBResultMapper doesnt call POJO setter sjoshid/influxdb-java
4 participants
@majst01 @fmachado @sjoshid @John9570