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.

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

[repository] [jdbc] SQL Server support #1772

Closed
Sudhanshus2020 opened this issue Dec 26, 2018 · 23 comments

Comments

@Sudhanshus2020
Copy link

commented Dec 26, 2018

Expected Behavior

Do you have any plan in Road-map to enable SQL Server support

Current Behavior

As of now it is not supporting SQL Server

@brasseld

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

Hi @Sudhanshus2020

Sql server support must probably be part of jdbc repository.
Did you had a look to it ? Perhaps you can provide such support ?

@Sudhanshus2020

This comment has been minimized.

Copy link
Author

commented Dec 27, 2018

HI @brasseld,

With existing JDBC component we are facing issues, main cause of issues with exiting code is, it is using key-words as columns name that's why component is not able to create Tables as well as not able to execute SQL.
Actually I am still not aware with the IMPACT of this change that's why I asked you guys about Road-Map.
If you guys can share the Schema/ER Diagram then we can figure out the impact and work out to enable SQL server support.

@brasseld

This comment has been minimized.

Copy link
Member

commented Dec 27, 2018

Hi @Sudhanshus2020

We are using Liquibase to manage the database schema.
Please have a look to https://github.com/gravitee-io/gravitee-repository-jdbc/tree/master/src/main/resources/liquibase

Perhaps you can share a stacktrace to see what are the reserved keywords for SQL server ?

@brasseld

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

Hi @Sudhanshus2020

Any news here ?

@brasseld brasseld changed the title Any Plan in Road-map to enable SQL Server support [repository] [jdbc] Any Plan in Road-map to enable SQL Server support Dec 28, 2018

@Sudhanshus2020

This comment has been minimized.

Copy link
Author

commented Dec 29, 2018

Hello @brasseld,
Apologies for late replay, here is the update...

I have gone through the code and DB Schema, we need to modify 3 things to enable SQL Server support:

  1. Need to provide provision to warp keywords with the escape character as per the DB provider, in case of SQL Server it is [].
  2. As some of the column name is the keywords that's why Liquibase is not able to create tables, so I have converted Liquibase file to SQL file and I am able to create require table.
  3. One thing which again require manipulation in code is, SQL server doesn't have direct Boolean datatype so we need to modify below code but also need to check how it will support backward compatibility.
    In below code we need to modify the where clause which is where async = true with where async = 'true', however I didn't check will this work with MySQL or not, I hope it should.
 //class JdbcRateLimitRepository
  public Iterator<RateLimit> findAsyncAfter(long timestamp) {
        final List<RateLimit> items = jdbcTemplate.query("select " + escapeReservedWord("key") + ", counter, last_request, reset_time, created_at, updated_at, async "
                        + " from ratelimit "
                        + " where async = true and updated_at > ?"
                , MAPPER
                , timestamp
        );
        return items.iterator();
    }

As of now after performing above modifications, I am able to consume the gateway, management-ui, management-core with SQL Server DB as well as Azure SQL Server.

Let me know what you thought, how we can implement these changes efficiently.

@brasseld

This comment has been minimized.

Copy link
Member

commented Dec 29, 2018

Hi @Sudhanshus2020

Can you tell us what are the keywords we have to protect ?
Did you try to update the escapeReservedWord() method to set the escape character for mssql? See https://github.com/gravitee-io/gravitee-repository-jdbc/blob/master/src/main/java/io/gravitee/repository/jdbc/common/AbstractJdbcRepositoryConfiguration.java#L108

About boolean column, and according to this post, liquibase is using the bit field type to handle boolean values. Is it the case for you ? Why did you map it to a string ('true')?
From my point of view (I do not know MSSQL very well) the mssql driver must handle and convert boolean column automatically to a bit column.

@brasseld

This comment has been minimized.

Copy link
Member

commented Dec 29, 2018

Ok, I think the findAsyncAfter() method should be refactor to something like this:

    @Override
    public Iterator<RateLimit> findAsyncAfter(long timestamp) {
        final List<RateLimit> items = jdbcTemplate.query("select " + escapeReservedWord("key") + ", counter, last_request, reset_time, created_at, updated_at, async "
                        + " from ratelimit "
                        + " where async = ? and updated_at > ?"
                , MAPPER
                , true
                , timestamp
        );
        return items.iterator();
    }

To let the driver doing its stuff (converting from boolean to bit).

WDYT ?

@Sudhanshus2020

This comment has been minimized.

Copy link
Author

commented Dec 31, 2018

Ok, I think the findAsyncAfter() method should be refactor to something like this:

    @Override
    public Iterator<RateLimit> findAsyncAfter(long timestamp) {
        final List<RateLimit> items = jdbcTemplate.query("select " + escapeReservedWord("key") + ", counter, last_request, reset_time, created_at, updated_at, async "
                        + " from ratelimit "
                        + " where async = ? and updated_at > ?"
                , MAPPER
                , true
                , timestamp
        );
        return items.iterator();
    }

To let the driver doing its stuff (converting from boolean to bit).

WDYT ?

Above recommendation will definitely work.

@Sudhanshus2020

This comment has been minimized.

Copy link
Author

commented Dec 31, 2018

Hi @Sudhanshus2020

Can you tell us what are the keywords we have to protect ?
Did you try to update the escapeReservedWord() method to set the escape character for mssql? See https://github.com/gravitee-io/gravitee-repository-jdbc/blob/master/src/main/java/io/gravitee/repository/jdbc/common/AbstractJdbcRepositoryConfiguration.java#L108

About boolean column, and according to this post, liquibase is using the bit field type to handle boolean values. Is it the case for you ? Why did you map it to a string ('true')?
From my point of view (I do not know MSSQL very well) the mssql driver must handle and convert boolean column automatically to a bit column.

Yes, we need to modify code as mentioned below:
Previous Code:

    private static char escapeReservedWordsChar = '`';

    public static String escapeReservedWord(final String word) {
        return escapeReservedWordsChar + word + escapeReservedWordsChar;
    }

    public static void setEscapeReservedWordFromJDBCUrl(final String jdbcUrl) {
        if (jdbcUrl != null && "postgresql".equals(jdbcUrl.split(":")[1])) {
            escapeReservedWordsChar = '\"';
        }
    }

New Code:

    private static char escapeReservedWordsPrefixChar = '`';    
    private static char escapeReservedWordsSufixChar = '`';
    
    public static String escapeReservedWord(final String word) {
        return escapeReservedWordsPrefixChar + word + escapeReservedWordsSufixChar;
    }

     public static void setEscapeReservedWordFromJDBCUrl(final String jdbcUrl) {
        if (jdbcUrl != null && "postgresql".equals(jdbcUrl.split(":")[1])) {
        	escapeReservedWordsPrefixChar = '\"';
        	escapeReservedWordsSufixChar = '\"';        	
        }else if (jdbcUrl != null && "sqlserver".equals(jdbcUrl.split(":")[1])) {
        	escapeReservedWordsPrefixChar = '[';
        	escapeReservedWordsSufixChar = ']';        	
        }
    }

In above recommendation, I have provisioned to escape reserve word if the prefix and suffix are distinct.

@brasseld

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

So, with this change, it is working as expected?
Please confirm

@Sudhanshus2020

This comment has been minimized.

Copy link
Author

commented Dec 31, 2018

So, with this change, it is working as expected?
Please confirm

Hi,
I just validated the plain scenario which are

  • Login with Admin
  • Create a keyless API

However to validate all the testcases it may take some time from my end.
FYI: I am going on holiday for this week, I can validate it after 7-Jan.

@brasseld

This comment has been minimized.

Copy link
Member

commented Dec 31, 2018

Sure, no problem @Sudhanshus2020

Let's do that when you will be back!
Enjoy your holidays ;)

brasseld pushed a commit to gravitee-io/gravitee-repository-jdbc that referenced this issue Dec 31, 2018

@Sudhanshus2020

This comment has been minimized.

Copy link
Author

commented Jan 7, 2019

Hi @brasseld,
In testing I have got some issues, please find below list:

  • There are 2 more keywords available in SQL which are plan & view, we also need to warp these keywords with escapeReservedWord method.

I have done above modification in my local code and after that, I am able to successfully validate end-2-end API configuration with OAuth2 using keycloak adapter and with keyless plan as well, in addition to this I have also applied DYNAMIC_ROUTING & TRANSFORM_HEADER policies in API.

Till now I have done 3 major modification in code, which are:

  1. Updated setEscapeReservedWordFromJDBCUrl method as mentioned in above trail.
  2. Updated findAsyncAfter as recommended by you
  3. Support plan & view keywords.

Should I send you merge request for above modification, WDYT ?
If there are any specific testcases you want me to execute let me know.

@brasseld

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Hi @Sudhanshus2020

Please tell me what you've done for 3) I will update source code from my branch.

@Sudhanshus2020

This comment has been minimized.

Copy link
Author

commented Jan 7, 2019

Hi @Sudhanshus2020

Please tell me what you've done for 3) I will update source code from my branch.

I have modified 3 files which is attached in the below zip.

Below are the file names which exist at \src\main\java\io\gravitee\repository\jdbc\management :

  • JdbcApiKeyRepository.java
  • JdbcApiRepository.java
  • JdbcSubscriptionRepository.java

For changes refer here ChangesForKeywords.zip

@brasseld

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Now the best would be to find an embedded version of SQLServer for Java to automatically run repository tests for sqlserver.

But I did not found any solution for this :(

@Sudhanshus2020

This comment has been minimized.

Copy link
Author

commented Jan 8, 2019

Now the best would be to find an embedded version of SQLServer for Java to automatically run repository tests for sqlserver.

But I did not found any solution for this :(

Even, I haven't found.

@brasseld

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

Meaning that we will not being able to automatically test sqlserver support from our CI :(

@Sudhanshus2020

This comment has been minimized.

Copy link
Author

commented Jan 16, 2019

Meaning that we will not being able to automatically test sqlserver support from our CI :(

Have you merge the code in master ?

@NicolasGeraud

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

hi @Sudhanshus2020 until we found a solution to be try sqlserver inside our build process, we are not able to merge this PR.

I include it in milestone 1.23 and add the label "in review". We will try to find a solution together.

@aelamrani

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

I changed the status to on hold til we find a solution to really review with integrated tests

@brasseld

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

One solution would be to play with https://www.testcontainers.org/ while running tests.

@brasseld brasseld removed this from the APIM - 1.23.0 milestone Feb 15, 2019

@Sudhanshus2020

This comment has been minimized.

Copy link
Author

commented Feb 16, 2019

One solution would be to play with https://www.testcontainers.org/ while running tests.

This is pretty cool 👍

NicolasGeraud added a commit to gravitee-io/gravitee-repository-jdbc that referenced this issue Mar 19, 2019

NicolasGeraud added a commit to gravitee-io/gravitee-repository-test that referenced this issue Mar 19, 2019

NicolasGeraud added a commit to gravitee-io/gravitee-repository-jdbc that referenced this issue Apr 9, 2019

NicolasGeraud added a commit to gravitee-io/gravitee-repository-test that referenced this issue Apr 9, 2019

@NicolasGeraud NicolasGeraud added this to the APIM - 1.25.0 milestone Apr 9, 2019

NicolasGeraud added a commit to gravitee-io/gravitee-repository-jdbc that referenced this issue Apr 9, 2019

aelamrani added a commit to gravitee-io/gravitee-repository-jdbc that referenced this issue Apr 10, 2019

aelamrani added a commit to gravitee-io/gravitee-docs that referenced this issue Apr 10, 2019

NicolasGeraud added a commit to gravitee-io/gravitee-repository-jdbc that referenced this issue Apr 10, 2019

NicolasGeraud added a commit to gravitee-io/gravitee-repository-jdbc that referenced this issue Apr 10, 2019

NicolasGeraud added a commit to gravitee-io/gravitee-repository-jdbc that referenced this issue Apr 10, 2019

aelamrani added a commit to gravitee-io/gravitee-repository-jdbc that referenced this issue Apr 11, 2019

aelamrani added a commit to gravitee-io/gravitee-docs that referenced this issue Apr 18, 2019

aelamrani added a commit to gravitee-io/gravitee-repository-test that referenced this issue Apr 18, 2019

@aelamrani aelamrani changed the title [repository] [jdbc] Any Plan in Road-map to enable SQL Server support [repository] [jdbc] SQL Server support Apr 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.