Menu

dbunit Merge Request #31: Add PostgreSQL OID data type (convert from/to byte[]) (rejected)

Merging...

Merged

Something went wrong. Please, merge manually

Checking if merge is possible...

Something went wrong. Please, merge manually

Hou Tianze wants to merge 1 commit from /u/ibic/dbunit/ to master, 2017-09-15

Just as we discussed over:
https://sourceforge.net/p/dbunit/feature-requests/179/

As I am not too familiar with the code structures/conventions of dbunit, here are the things you may want to note:

  • I modifed the Maven file dbunit/pom.xml, to add postgresql dependency. I'm not sure if this extra dependency is necessary (as it already exists in some other section(s)), or the place of adding, but I can't compile without this addtion.
  • I added PostgreSQLOidDataType.java to package org.dbunit.dataset.datatype instead of org.dbunit.ext.postgresql, because the class PostgreSQLOidDataType extends BytesDataType, which is in the datatype package, due to accessibility, PostgreSQLOidDataType has to reside in the same package as well. As Java doesn't have a friend keyword, I couldn't find a better way to refactor this.

Needless to say, major implmentation code were done the original author @mfrechePgest .

Commit Date  
[d4ab0b] by Hou Tianze Hou Tianze

Add PostgreSQL OID data type (convert from/to byte[])

2017-09-06 10:02:42 Tree

Discussion

  • Jeff Jensen

    Jeff Jensen - 2017-09-09

    Very nice, thank you!

    A couple of questions:

    1. Is this still a TODO or is it a note and we should remove the "TODO: word: "// TODO: I don't know if the following zero check..."?
    2. PostgreSQLOidDataType has a TODO "// TODO: I need to do this check to avoid exceptions as well", is this a leftover and we should remove it or does it mean a check needs adding?

    A few code adjustment requests: Are you ok to do these and squash into one commit? (then it's fully your commit vs yours and my mods).

    1. Make the BytesDataType constructor public then can move PostgreSQLOidDataType to the org.dbunit.ext.postgresql package. Idk why it's not public; its parent and other similar classes have public constructors so let's make it public.
    2. Since you have a real use case and knowledge, would you mind adding an integration test for an oid field? It involves adding to SQLHelperDomainPostgreSQLIT.
    3. In the same commit add this to the changes.xml file (replace the current empty "in scm" one):
        <release version="in scm" date="next" description="PostgreSQL OID, ">
          <action dev="jeffjensen" type="add" issue="179" due-to="ibic">Add PostgreSQL OID data type.</action>
        </release>
    

    Any thoughts/questions, just let me know!

     
    • Hou Tianze

      Hou Tianze - 2017-09-15

      You're welcome.

      1. It's a note for the merger, I will check them again and remove after confirming.
      2. Same a point 1

      For the adjustments:

      1. Will do that, I also think that package is more suitable for the class
      2. OK, I will look into that SQLHelperDomainPostgreSQLIT class. Do you have a short intro how to write/run integration tests?
      3. OK, will add. How do I add the original author of the code to change.xml, using a comma like the following?
        due-to="mfrechePgest,ibic"

      One more question: I guess there are probably changes in your upstream repository, do I need to pull & rebase in my repo to make the changes? Or I just simpley continue working on my repo, sqash the commits into one and raise another merge-request? Thanks.

       

      Last edit: Hou Tianze 2017-09-15
      • Jeff Jensen

        Jeff Jensen - 2017-09-15
        1. Great, thank you!
        2. Make a test method in that class similar to xtestDomainDataTypes - use the statement to run DDL to create table(s) as needed, use dbUnit methods to setup and run test, do asserts. Keep it simple and straight-forward that way!
        3. Good idea, use this approach:
            <release version="in scm" date="next" description="PostgreSQL OID">
              <action dev="jeffjensen" type="add" issue="179" due-to="mfrechePgest">
                Add PostgreSQL OID data type.
                <dueto name="ibic"/>
              </action>
            </release>
        

        Yes, pull in anything latest so the final MR is clean/easy. May not be any commits since you started.

         

        Last edit: Jeff Jensen 2017-09-15
        • Hou Tianze

          Hou Tianze - 2017-09-15

          Thanks for the info. I've created the integration test (and the test passed) and made another merge-request here:
          https://sourceforge.net/p/dbunit/code.git/merge-requests/33/

          You can ignore its previous merge-request (32), in which I forgot to remove PostgreSQLOidDataTypefrom the wrong location.

           
  • Hou Tianze

    Hou Tianze - 2017-09-15
    • Status: open --> rejected
     

Log in to post a comment.

MongoDB Logo MongoDB