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

Display JDBC driver name during startup #378

Closed
wants to merge 4 commits into from
Closed

Display JDBC driver name during startup #378

wants to merge 4 commits into from

Conversation

apeiris
Copy link
Contributor

@apeiris apeiris commented May 28, 2024

Display JDBC driver name

apeiris added 4 commits May 27, 2024 21:45
- Updated Server\pom.xml to include mssql-jdbc 12.6.2.jre11.
  This driver has some vulnerabilities, update as soon as they are resolved
- Updated Server\config\config.xml to support MSSQL.
…e(?,?) for improved generality

- The H2 database file Server/src/config/db/openas2.mv.db now includes the routine getMessagesByDateRange
Copy link
Contributor

@uhurusurfa uhurusurfa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR.
I have made comments below that will need to be made before this PR can be approved.

import java.sql.SQLException;
import java.sql.Statement;
import java.sql.Types;
import java.sql.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have specifically avoided using asterisk imports in OpenAS2 for various reasons so please restore this to specific imports. Search the internet for reasons why not to use asterisk if you are interested in understanding why.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no functional change to this file. Revert to HEAD for this PR.

Comment on lines +285 to +288
CallableStatement cs = conn.prepareCall("{CALL ftGetMessagesByDateRange(?,?)}");
cs.setDate(1, Date.valueOf(map.get("startDate")));
cs.setDate(2,Date.valueOf(map.get("endDate")));
ResultSet rs = cs.executeQuery();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is your own custom callable.
It will not be added to OpenAS2 since the goal is to remain generic.
I assume the statement that is currently used does not work on SqlServer because it does not support the TIMESTAMP operator so instead change the statement to this and it will work on all SQL-92 and newer standards compliant database servers:

            ResultSet rs = s.executeQuery("SELECT " + FIELDS.MSG_ID + ",STATE,STATUS,CREATE_DT FROM " + tableName
                    + " WHERE CREATE_DT BETWEEN CAST('" + map.get("startDate").toString()
                    + " 00:00:00' as TIMESTAMP) AND CAST('" + map.get("endDate").toString()
                    + " 23:59:59'" as TIMESTAMP));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no functional change to this file. Revert to HEAD for this PR.

Comment on lines +301 to +306
<!-- https://mvnrepository.com/artifact/com.microsoft.sqlserver/mssql-jdbc -->
<dependency>
<groupId>com.microsoft.sqlserver</groupId>
<artifactId>mssql-jdbc</artifactId>
<version>12.6.2.jre11</version>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this import as it loads a library into the installler package that most people will never use and is not part of the default OpenAS2 functionality.
Anyone wishing to use packages other than the standard packages provided by OpenAS2 should create their own wrapper installer otherwise the OpenAS2 install package will become bloated unnecessarily.

@apeiris apeiris closed this by deleting the head repository Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants