GeoTools
  1. GeoTools
  2. GEOT-3175

Map parameters in OracleNGDataStoreFactory

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.6.4
    • Fix Version/s: 2.6.6
    • Component/s: jdbc
    • Labels:
      None
    • Environment:
      windows vista, jdk1.5.0_22

      Description

      Hi,

      I need to access an Oracle data store using the following parameters:

      USER: xxx

      PASSWD: xxx

      DATABASE: (DESCRIPTION=(ADDRESS_LIST = (ADDRESS = (PROTOCOL = TCP)(HOST = xxx)(PORT = xxxx))(ADDRESS = (PROTOCOL = TCP)(HOST = xxx)(PORT = xxxx))(LOAD_BALANCE = yes))(CONNECT_DATA=(SERVER=DEDICATED)(SERVICE_NAME=xxx.xxx.xxx.xx)(FAILOVER_MODE=(TYPE=SELECT)(METHOD=BASIC)(RETRIES=180)(DELAY=5))))

      When you run the OracleNGDataStoreFactory.getJDBCUrl (Map params) protected method, the execution of the 'lookUp' function throws an IOException as the HOST and PORT parameters are required.

      This is the method code:

      ...

      @Override
      protected String getJDBCUrl(Map params) throws IOException

      { String host = (String) HOST.lookUp(params); String db = (String) DATABASE.lookUp(params); int port = (Integer) PORT.lookUp(params); if( db.startsWith("(") ) return JDBC_PATH + db; else if( db.startsWith("/") ) return JDBC_PATH + "//" + host + ":" + port + db; else return JDBC_PATH + host + ":" + port + ":" + db; }

      ...

      In this method if the DATABASE parameter starts with "(", the HOST and PORT parameters are not used because the function returns 'JDBC_PATH + db'.

      I have consulted the Oracle documentation at this link:

      http://www.oracle.com/technology/tech/java/sqlj_jdbc/htdocs/jdbc_faq.html#top (in the 'Connections' section)

      and I see that the HOST and PORT parameters are not always required to compose the JDBC URL.

      In the current 'getJDBCUrl' method the DATABASE parameter is alwais required to compose the JDBC URL but, in the JDBCDataStoreFactory
      class, this parameter not have the "required" flag set up.

      To resolve this problem I modified the OracleNGDataStoreFactory class as follow:

      1) Marking HOST and PORT parameters as not "required" and DATABASE as "required":

      ...

      /** parameter for database port */
      public static final Param PORT = new Param("port", Integer.class, "Port", false, 1521);

      /** parameter for database host */
      public static final Param HOST = new Param("host", String.class, "Host", false, "localhost");

      /** parameter for database instance */
      public static final Param DATABASE = new Param("database", String.class, "Database", true);

      ...

      2) Modifying 'getJDBCurl' method as follow:

      ...

      @Override
      protected String getJDBCUrl(Map params) throws IOException

      { String db = (String) DATABASE.lookUp(params); String host = (String) HOST.lookUp(params); Integer port =(Integer) PORT.lookUp(params); if(db.startsWith("(")) return JDBC_PATH.concat(db); else if(db.startsWith("/") && host != null && port != null) return JDBC_PATH.concat("//").concat(host).concat(":").concat(port.toString()).concat(db); else if(host != null && port != null) return JDBC_PATH.concat(host).concat(":").concat(port.toString()).concat(":").concat(db); else throw new IOException("Unable to properly compose the JDBC URL string, some parameters as host and port may be null !"); }

      ...

      3) Modifying the 'setupParameters' method introducing the new HOST, PORT and DATABASE definitions:

      ...

      @Override
      protected void setupParameters(Map parameters)

      { // NOTE: when adding parameters here remember to add them to OracleNGOCIDataStoreFactory and // OracleNGJNDIDataStoreFactory super.setupParameters(parameters); parameters.put(LOOSEBBOX.key, LOOSEBBOX); parameters.put(MAX_OPEN_PREPARED_STATEMENTS.key, MAX_OPEN_PREPARED_STATEMENTS); parameters.put(PORT.key, PORT); parameters.put(HOST.key, HOST); parameters.put(DATABASE.key, DATABASE); parameters.put(DBTYPE.key, DBTYPE); }

      ...

      Regards

        Activity

        Hide
        Andrea Aime added a comment -
        The patch looks almost fine.
        I wonder why you people cannot just use "+" to concatenate strings and always has to reach to less readable ways to do the work. Btw, the one you used is _less_ efficient than using "+" as far as I know.
        Please revert those changes
        Show
        Andrea Aime added a comment - The patch looks almost fine. I wonder why you people cannot just use "+" to concatenate strings and always has to reach to less readable ways to do the work. Btw, the one you used is _less_ efficient than using "+" as far as I know. Please revert those changes
        Hide
        Tobia Di Pisa added a comment -
        Reverted changes to string concatenation.
        Show
        Tobia Di Pisa added a comment - Reverted changes to string concatenation.
        Hide
        Andrea Aime added a comment -
        Patch looks good. Do you have commit rights?
        Show
        Andrea Aime added a comment - Patch looks good. Do you have commit rights?
        Hide
        Tobia Di Pisa added a comment -
        I need to commit stuff on JDBC and Oracle modules concerning JIRAs GEOT-3175 and GEOT-3176, but actually I do not have commit rights on that modules and GeoTools at all. Therefore I would like to ask to the community commit rights on GeoTools repositories.
        Show
        Tobia Di Pisa added a comment - I need to commit stuff on JDBC and Oracle modules concerning JIRAs GEOT-3175 and GEOT-3176 , but actually I do not have commit rights on that modules and GeoTools at all. Therefore I would like to ask to the community commit rights on GeoTools repositories.
        Hide
        Andrea Aime added a comment -
        You have to do so on the geotools-devel mailing list. Once you got them you'll still need to ask for review from the module maintainer before committing any patch (for the Oracle module, I'm the one)
        Show
        Andrea Aime added a comment - You have to do so on the geotools-devel mailing list. Once you got them you'll still need to ask for review from the module maintainer before committing any patch (for the Oracle module, I'm the one)
        Hide
        Andrea Aime added a comment -
        Btw, better introduce yourself, what you do, company you work for etc, people in the community wants to know who's got commit rights ;-)
        Show
        Andrea Aime added a comment - Btw, better introduce yourself, what you do, company you work for etc, people in the community wants to know who's got commit rights ;-)
        Hide
        Ben Caradoc-Davies added a comment -
        Won't making the new database parameter required break backwards compatibility? What happens to existing applications that have serialised parameters storing only host and port and not database? Making database required will make these configurations invalid and cause connections to fail if the software is updated. This is an incompatible API change.
        Show
        Ben Caradoc-Davies added a comment - Won't making the new database parameter required break backwards compatibility? What happens to existing applications that have serialised parameters storing only host and port and not database? Making database required will make these configurations invalid and cause connections to fail if the software is updated. This is an incompatible API change.
        Hide
        Andrea Aime added a comment -
        Ben, it is only if the host + port and no database name produced a valid connection string. Did it? I'm not so expert in Oracle connections, I never connected without specifying the db name.

        I'm looking online for an answer but I'm not finding much. Here there is something:
        http://www.oracle.com/technology/tech/java/sqlj_jdbc/htdocs/jdbc_faq.html#05_03

        and they don't say database is explicit, whilst username/password are explicitly declared as optional.
        Show
        Andrea Aime added a comment - Ben, it is only if the host + port and no database name produced a valid connection string. Did it? I'm not so expert in Oracle connections, I never connected without specifying the db name. I'm looking online for an answer but I'm not finding much. Here there is something: http://www.oracle.com/technology/tech/java/sqlj_jdbc/htdocs/jdbc_faq.html#05_03 and they don't say database is explicit, whilst username/password are explicitly declared as optional.
        Hide
        Ben Caradoc-Davies added a comment -
        Andrea, I don't know enough about Oracle either. My remark was just an observation that the patch, while doing what looks like the right thing (overriding the behaviour of the parent class setupParameters), might introduce an incompatibility. Do we have an Oracle experts who could advise whether it is possible to connect without a database set? I find Oracle SID and SERVICE quite confusing.
        Show
        Ben Caradoc-Davies added a comment - Andrea, I don't know enough about Oracle either. My remark was just an observation that the patch, while doing what looks like the right thing (overriding the behaviour of the parent class setupParameters), might introduce an incompatibility. Do we have an Oracle experts who could advise whether it is possible to connect without a database set? I find Oracle SID and SERVICE quite confusing.
        Hide
        Andrea Aime added a comment -
        Nope, don't know of any Oracle expert that could help. My take is, we let the patch go as is, and if it breaks someone, we ask where were they when their help was needed.
        Once we've done what we could we should not ask ourselves for more.
        Show
        Andrea Aime added a comment - Nope, don't know of any Oracle expert that could help. My take is, we let the patch go as is, and if it breaks someone, we ask where were they when their help was needed. Once we've done what we could we should not ask ourselves for more.
        Hide
        Tobia Di Pisa added a comment -
        Is it possible to commit the patch? If yes, Andrea can do it for me? I have no commit rights.
        Show
        Tobia Di Pisa added a comment - Is it possible to commit the patch? If yes, Andrea can do it for me? I have no commit rights.
        Hide
        Andrea Aime added a comment -
        Sure I can. So you haven't sent the contribution agreement yet?
        Show
        Andrea Aime added a comment - Sure I can. So you haven't sent the contribution agreement yet?
        Hide
        Tobia Di Pisa added a comment -
        Not yet, I will send the contribution agreement this week.
        Show
        Tobia Di Pisa added a comment - Not yet, I will send the contribution agreement this week.
        Hide
        Ben Caradoc-Davies added a comment -
        Tobia, we are still waiting for two more votes from existing committers before you are granted access. I'll nag the list. ;-)

        It is my strong preference that you commit this patch yourself to build the capability of the community.
        Show
        Ben Caradoc-Davies added a comment - Tobia, we are still waiting for two more votes from existing committers before you are granted access. I'll nag the list. ;-) It is my strong preference that you commit this patch yourself to build the capability of the community.
        Hide
        Andrea Aime added a comment -
        Ben, I'll be committing his patch today as he asked me to do so, he's late in the copyright assignement paperwork. Hopefully by next time he'll have it done.
        Show
        Andrea Aime added a comment - Ben, I'll be committing his patch today as he asked me to do so, he's late in the copyright assignement paperwork. Hopefully by next time he'll have it done.
        Hide
        Ben Caradoc-Davies added a comment -
        Andrea, you committing the patch is no different from Tobia committing the patch, without paperwork, so why not let him commit it? This is an opportunity to induct a new developer.
        Show
        Ben Caradoc-Davies added a comment - Andrea, you committing the patch is no different from Tobia committing the patch, without paperwork, so why not let him commit it? This is an opportunity to induct a new developer.
        Hide
        Andrea Aime added a comment -
        I agree but only to a point. We'd be creating a dangerous precedent. We always accepted small patches without signign the contribution agreement, but never gave commit access without one. It's a slippery slope, don't want to get into it, plus, one needs some incentive to sign and send the contributor agreement no?
        Show
        Andrea Aime added a comment - I agree but only to a point. We'd be creating a dangerous precedent. We always accepted small patches without signign the contribution agreement, but never gave commit access without one. It's a slippery slope, don't want to get into it, plus, one needs some incentive to sign and send the contributor agreement no?
        Hide
        Ben Caradoc-Davies added a comment -
        OK, if Tobia asked you to commit the patch, then please go ahead and do so.
        Show
        Ben Caradoc-Davies added a comment - OK, if Tobia asked you to commit the patch, then please go ahead and do so.
        Hide
        Tobia Di Pisa added a comment -
        Patch committed in r36162 on trunk and r36160 on 2.6.x.
        Show
        Tobia Di Pisa added a comment - Patch committed in r36162 on trunk and r36160 on 2.6.x.
        Hide
        Andrea Aime added a comment -
        Closing the jira, commits already happened
        Show
        Andrea Aime added a comment - Closing the jira, commits already happened
        Hide
        Andrea Aime added a comment -
        Mass closing all issues in resolved state that have not been reopened nor commented over in the last month
        Show
        Andrea Aime added a comment - Mass closing all issues in resolved state that have not been reopened nor commented over in the last month

          People

          • Assignee:
            Andrea Aime
            Reporter:
            Tobia Di Pisa
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: