isRecordID NPE if using absolute (absolute should should move also currentRecord) #9

Closed
SvenNold opened this Issue Sep 3, 2014 · 13 comments

Comments

Projects
None yet
2 participants
@SvenNold

SvenNold commented Sep 3, 2014

Hi

sorry for using this contact but the issue tracker on sourceforge looks pretty dead. I forced an update for camel-flatpack component to directly use flatpack jar from maven central (before they used servicemix wrapper).
But the test cases fail, I identified a change on DefaultDataSet, which causes this error.

In 3.2.0 it was

public boolean isRecordID(String recordID)
  {
    String rowID = ((Row)this.rows.get(this.pointer)).getMdkey();
    if (rowID == null) {
      rowID = "detail";
    }
    return rowID.equals(recordID);
  }

In 3.4.1 it is:

public boolean isRecordID(final String recordID) {
        return currentRecord.isRecordID(recordID);
    }

Problem is that in method absolute the pointer gets changed, but not the currentRecord, which causes a NPE.

Could you provide a fix so that I can push forward to use flatpack 3.4.X for the camel-flatpack component.

@benoitx

This comment has been minimized.

Show comment
Hide comment
@benoitx

benoitx Sep 3, 2014

Contributor

Thanks for raising this. I'll investigate (never user the absolute method). Would you have a unit test that could show this and I'll use it as a basis for the patch. Many thanks

Contributor

benoitx commented Sep 3, 2014

Thanks for raising this. I'll investigate (never user the absolute method). Would you have a unit test that could show this and I'll use it as a basis for the patch. Many thanks

@benoitx benoitx added the bug label Sep 3, 2014

@benoitx benoitx self-assigned this Sep 3, 2014

@SvenNold

This comment has been minimized.

Show comment
Hide comment
@SvenNold

SvenNold Sep 3, 2014

Please use the current camel-flatpack (https://github.com/apache/camel/tree/master/components/camel-flatpack) you need to modify the pom.xml to use your lib:

    <dependency>
  <groupId>net.sf.flatpack</groupId>
  <artifactId>flatpack</artifactId>
  <version>3.4.1</version>
</dependency>

Instead of the service mix one.

SvenNold commented Sep 3, 2014

Please use the current camel-flatpack (https://github.com/apache/camel/tree/master/components/camel-flatpack) you need to modify the pom.xml to use your lib:

    <dependency>
  <groupId>net.sf.flatpack</groupId>
  <artifactId>flatpack</artifactId>
  <version>3.4.1</version>
</dependency>

Instead of the service mix one.

@benoitx

This comment has been minimized.

Show comment
Hide comment
@benoitx

benoitx Sep 3, 2014

Contributor

Would this change to absolute fix the issue you think:

Thanks

    /**
     * Sets the absolute position of the record pointer
     *
     * @param localPointer
     *            - int
     * @exception IndexOutOfBoundsException
     */
    @Override
    public void absolute(final int localPointer) {
        if (localPointer < 0 || localPointer > rows.size() - 1) {
            throw new IndexOutOfBoundsException("INVALID POINTER LOCATION: " + localPointer);
        }

        pointer = localPointer;
        currentRecord = new RowRecord(rows.get(pointer), metaData, parser.isColumnNamesCaseSensitive(), pzConvertProps, strictNumericParse,
                upperCase, lowerCase, parser.isNullEmptyStrings());
    }
Contributor

benoitx commented Sep 3, 2014

Would this change to absolute fix the issue you think:

Thanks

    /**
     * Sets the absolute position of the record pointer
     *
     * @param localPointer
     *            - int
     * @exception IndexOutOfBoundsException
     */
    @Override
    public void absolute(final int localPointer) {
        if (localPointer < 0 || localPointer > rows.size() - 1) {
            throw new IndexOutOfBoundsException("INVALID POINTER LOCATION: " + localPointer);
        }

        pointer = localPointer;
        currentRecord = new RowRecord(rows.get(pointer), metaData, parser.isColumnNamesCaseSensitive(), pzConvertProps, strictNumericParse,
                upperCase, lowerCase, parser.isNullEmptyStrings());
    }
@SvenNold

This comment has been minimized.

Show comment
Hide comment
@SvenNold

SvenNold Sep 3, 2014

At least it seems to pass the tests. Could you please provide a 3.4.2 version on maven central, so I could get this version proposed for next version 2.14.0 of camel.

SvenNold commented Sep 3, 2014

At least it seems to pass the tests. Could you please provide a 3.4.2 version on maven central, so I could get this version proposed for next version 2.14.0 of camel.

@benoitx

This comment has been minimized.

Show comment
Hide comment
@benoitx

benoitx Sep 3, 2014

Contributor

I will try to do a release soon. Might have to wait a couple of days.

Thanks

Contributor

benoitx commented Sep 3, 2014

I will try to do a release soon. Might have to wait a couple of days.

Thanks

@benoitx

This comment has been minimized.

Show comment
Hide comment
@benoitx

benoitx Sep 3, 2014

Contributor

oh, you want a 3.4.2 (i.e. NOT JDK8? which would be 4.0.1)

Contributor

benoitx commented Sep 3, 2014

oh, you want a 3.4.2 (i.e. NOT JDK8? which would be 4.0.1)

benoitx added a commit that referenced this issue Sep 3, 2014

@benoitx benoitx added the in progress label Sep 3, 2014

benoitx added a commit that referenced this issue Sep 3, 2014

@SvenNold

This comment has been minimized.

Show comment
Hide comment
@SvenNold

SvenNold Sep 3, 2014

3.4.2 would be great.
I will slowly start the discussion about integrating the 4.0.1 with jdk8 support, which won't be such a p-i-a as soon camel-flatpack directly references your lib ;)

Not too much changes at once otherwise it gets declined.

SvenNold commented Sep 3, 2014

3.4.2 would be great.
I will slowly start the discussion about integrating the 4.0.1 with jdk8 support, which won't be such a p-i-a as soon camel-flatpack directly references your lib ;)

Not too much changes at once otherwise it gets declined.

@SvenNold

This comment has been minimized.

Show comment
Hide comment
@benoitx

This comment has been minimized.

Show comment
Hide comment
@benoitx

benoitx Sep 3, 2014

Contributor

Yes, the Travis-CI changes were in master.

This is now ready

https://oss.sonatype.org/content/repositories/snapshots/net/sf/flatpack/flatpack/3.4.2-SNAPSHOT/

Please give it a go

many thanks

B

Contributor

benoitx commented Sep 3, 2014

Yes, the Travis-CI changes were in master.

This is now ready

https://oss.sonatype.org/content/repositories/snapshots/net/sf/flatpack/flatpack/3.4.2-SNAPSHOT/

Please give it a go

many thanks

B

@SvenNold

This comment has been minimized.

Show comment
Hide comment
@SvenNold

SvenNold Sep 4, 2014

Great!!:

Tests run: 36, Failures: 0, Errors: 0, Skipped: 0

Please release it mvn central so that I can proceed.

Thanks

SvenNold commented Sep 4, 2014

Great!!:

Tests run: 36, Failures: 0, Errors: 0, Skipped: 0

Please release it mvn central so that I can proceed.

Thanks

@benoitx

This comment has been minimized.

Show comment
Hide comment
@benoitx

benoitx Sep 4, 2014

Contributor

Hi

I believe that it is release in Central now.

Kindly let me know

Thanks

Contributor

benoitx commented Sep 4, 2014

Hi

I believe that it is release in Central now.

Kindly let me know

Thanks

@SvenNold

This comment has been minimized.

Show comment
Hide comment
@SvenNold

SvenNold Sep 6, 2014

Thanks that was very fast. I already got CAMEL-7776 resolved, starting from camel v2.14 flatpack will be directly referenced. Nice job again!

-------- Ursprüngliche Nachricht --------
Von: Benoit Xhenseval notifications@github.com
Datum: 04.09.2014 17:49 (GMT+01:00)
An: Appendium/flatpack flatpack@noreply.github.com
Cc: Sven Nold Sven.Nold@isb-ag.de
Betreff: Re: [flatpack] isRecordID NPE if using absolute (absolute should should move also currentRecord) (#9)

Hi

I believe that it is release in Central now.

Kindly let me know

Thanks

Reply to this email directly or view it on GitHubhttps://github.com/Appendium/flatpack/issues/9#issuecomment-54486530.

SvenNold commented Sep 6, 2014

Thanks that was very fast. I already got CAMEL-7776 resolved, starting from camel v2.14 flatpack will be directly referenced. Nice job again!

-------- Ursprüngliche Nachricht --------
Von: Benoit Xhenseval notifications@github.com
Datum: 04.09.2014 17:49 (GMT+01:00)
An: Appendium/flatpack flatpack@noreply.github.com
Cc: Sven Nold Sven.Nold@isb-ag.de
Betreff: Re: [flatpack] isRecordID NPE if using absolute (absolute should should move also currentRecord) (#9)

Hi

I believe that it is release in Central now.

Kindly let me know

Thanks

Reply to this email directly or view it on GitHubhttps://github.com/Appendium/flatpack/issues/9#issuecomment-54486530.

@benoitx

This comment has been minimized.

Show comment
Hide comment
@benoitx

benoitx Sep 6, 2014

Contributor

Thank you for raising the issue.

I'll be releasing 4.0 soon

Benoit


Important Notice
This communication contains information that is considered
confidential and may also be privileged. It is for the exclusive use
of the intended
recipient(s). If you are not the intended recipient(s) please note that any
form of distribution, copying or use of this communication or the
information in it is strictly prohibited and may be unlawful. If you
have received
this communication in error please return it to the sender and
delete the original

On 6 Sep 2014, at 17:30, SvenNold notifications@github.com wrote:

Thanks that was very fast. I already got CAMEL-7776 resolved, starting from
camel v2.14 flatpack will be directly referenced. Nice job again!

-------- Ursprüngliche Nachricht --------
Von: Benoit Xhenseval notifications@github.com
Datum: 04.09.2014 17:49 (GMT+01:00)
An: Appendium/flatpack flatpack@noreply.github.com
Cc: Sven Nold Sven.Nold@isb-ag.de
Betreff: Re: [flatpack] isRecordID NPE if using absolute (absolute should
should move also currentRecord) (#9)

Hi

I believe that it is release in Central now.

Kindly let me know

Thanks

Reply to this email directly or view it on GitHub<
https://github.com/Appendium/flatpack/issues/9#issuecomment-54486530>.


Reply to this email directly or view it on GitHub
#9 (comment).

Contributor

benoitx commented Sep 6, 2014

Thank you for raising the issue.

I'll be releasing 4.0 soon

Benoit


Important Notice
This communication contains information that is considered
confidential and may also be privileged. It is for the exclusive use
of the intended
recipient(s). If you are not the intended recipient(s) please note that any
form of distribution, copying or use of this communication or the
information in it is strictly prohibited and may be unlawful. If you
have received
this communication in error please return it to the sender and
delete the original

On 6 Sep 2014, at 17:30, SvenNold notifications@github.com wrote:

Thanks that was very fast. I already got CAMEL-7776 resolved, starting from
camel v2.14 flatpack will be directly referenced. Nice job again!

-------- Ursprüngliche Nachricht --------
Von: Benoit Xhenseval notifications@github.com
Datum: 04.09.2014 17:49 (GMT+01:00)
An: Appendium/flatpack flatpack@noreply.github.com
Cc: Sven Nold Sven.Nold@isb-ag.de
Betreff: Re: [flatpack] isRecordID NPE if using absolute (absolute should
should move also currentRecord) (#9)

Hi

I believe that it is release in Central now.

Kindly let me know

Thanks

Reply to this email directly or view it on GitHub<
https://github.com/Appendium/flatpack/issues/9#issuecomment-54486530>.


Reply to this email directly or view it on GitHub
#9 (comment).

@benoitx benoitx closed this Dec 9, 2014

@benoitx benoitx removed the in progress label Dec 9, 2014

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