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

ISSUE-680 # Add the DB SQL Executor to import data from CSV and execute SQL statements #686

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

javiertuya
Copy link
Collaborator

@javiertuya javiertuya commented Oct 8, 2024

Add the DB SQL Executor to import data from CSV and execute SQL statements

Fixes Issue

PR Branch
https://github.com/javiertuya/zerocode/tree/680-db-sql-executor

Documentation at PR authorjapps/zerocode-tdd-docs#24

Motivation and Context

This PR provides an out of the box component that uses the Java API to execute steps that interact with a database.

The executor class org.jsmart.zerocode.core.db.DbSqlExecutor provides two operations:

  • EXECUTE: Run a SQL statement to verify the data stored in the database or insert/update in the data.
  • LOADCSV: Load the contents of a CSV file located in the resources folder into a table.

Acceptance critera fulfillment:

  • AC1, AC2, AC5, AC6: Done, with unit and integration tests, H2 database.
  • AC3: Done, PostgreSQL: the same unit and integration tests were executed in local, not in CI
  • AC4: Partially done, read CSV files from src/test/resources. Reading files from URL to be implemented in separate issue

Suggestions for future improvements:

  • Load from an url (AC4)
  • Load from sites that require authentication (mentioned in the issue)
  • Load from anywhere in the filesystem
  • Custom datetime formats (specified in the properties file or in the request)
  • Load data into binary columns (e.g. hexadecimal, base64...)
  • Run the PostgreSQL tests as part of the GitHub Actions workflow

Checklist:

  • New Unit tests were added

    • Covered in existing Unit tests
  • Integration tests were added

    • Covered in existing Integration tests
  • Test names are meaningful

  • Feature manually tested and outcome is successful

  • PR doesn't break any of the earlier features for end users

    • WARNING! This might break one or more earlier earlier features, hence left a comment tagging all reviewrs
  • Branch build passed in CI

  • No 'package.*' in the imports

  • Relevant DOcumentation page added or updated with clear instructions and examples for the end user

    • Not applicable. This was only a code refactor change, no functional or behaviourial changes were introduced
  • Http test added to http-testing module(if applicable) ?

    • Not applicable. The changes did not affect HTTP automation flow
  • Kafka test added to kafka-testing module(if applicable) ?

    • Not applicable. The changes did not affect Kafka automation flow

@nirmalchandra
Copy link
Collaborator

nirmalchandra commented Oct 8, 2024

@javiertuya , excellent work!
I've given the CI run and awaiting for a Green(check status here).

Just checking:

  1. Did you get chance to run a Integration-test scenario against a real Postgres DB (as the CI tests are using h2db) ?
    • Even Docker should be fine I guess, as this will validate the JDBC driver aspects ok
  2. Is it possible for you(if you by chance have an Infra with any DB) to run against a remote DB instance (example: RDS ) to check the Drivers work as expected?

@javiertuya
Copy link
Collaborator Author

@nirmalchandra

  1. Did you get chance to run a Integration-test scenario against a real Postgres DB (as the CI tests are using h2db) ?

Yes. The target DB is configured in core/src/test/resources/db_test.properties (this file has some instructions embedded). I ran both unit and integration tests using three different versions of Postgres. The only difference is that Postgress stores column and table identifiers as lowercase, so that, to make the integration tests pass, you need convert the json scenarios to have the key values in the verify to lowercase (this is done from the junit in the case of unit tests). For example, to run the integration test that loads a table from a CSV file with headers, you can do this case conversion by running from the db subfolder:

sed -i 's/"ID"/"id"/' db_csv_load_with_headers.json
sed -i 's/"NAME"/"name"/' db_csv_load_with_headers.json
sed -i 's/"AGE"/"age"/' db_csv_load_with_headers.json
  • Even Docker should be fine I guess, as this will validate the JDBC driver aspects ok

It would also be possible to run the postgres tests in the Github Actions (spin-up postgres takes around 6 seconds)

  1. Is it possible for you(if you by chance have an Infra with any DB) to run against a remote DB instance (example: RDS ) to check the Drivers work as expected?

I just checked it: connecting from a desktop to a remote server in my lab (through a VPN) and it works fine. To check this, just run a postgress instance in the server, and form the desktop, change the db.driver.url property to put the server ip and port

@authorjapps
Copy link
Owner

@a1shadows , @omkar-shitole 👋 :
Can you guys also have a look at the PR and review it?
Also if possible, check against some other real DB e.g. real MySQL etc?

@a1shadows
Copy link
Collaborator

Should we also try and create some documentation to https://github.com/authorjapps/zerocode-tdd-docs/tree/main?
Otherwise, feature looks good to me. I'll try and run it on my local this weekend

@authorjapps
Copy link
Owner

Should we also try and create some documentation to https://github.com/authorjapps/zerocode-tdd-docs/tree/main? Otherwise, feature looks good to me. I'll try and run it on my local this weekend

@a1shadows ,
It's already here.

But, please check if you want to add anything to it or amend it to make it more helpful, then please proceed 👍

@javiertuya
Copy link
Collaborator Author

@a1shadows , @omkar-shitole 👋 :
Can you guys also have a look at the PR and review it?
Also if possible, check against some other real DB e.g. real MySQL etc?

I've tested against MySQL. To run the tests, it needs a few changes:

  • A customization in tests due to MySQL particularities (e.g. ORDER BY ID NULLS FIRST must be made differently)
  • Test database setup: MySQL driver does not support running multiple SQLs (separted by ;) in a single statement. This will require (1) to separate the setup SQL statements, and, (2) allow an array of SQLs in the EXECUTE operation to perform all setup in a single step.

I can do the above changes in new commits in this PR or after review and merge, as you prefer.

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.

[DB] Import or load data from an external CSV file to a table
4 participants