Skip to content

Conversation

@h4shk4t
Copy link

@h4shk4t h4shk4t commented May 20, 2023

Adding a Redis as a new datasource to the framework. I have added its support to the main library as well as added an example.

@juntao
Copy link
Member

juntao commented May 20, 2023

Thanks. Can you also add a test case in GitHub Actions? 🙏

@h4shk4t
Copy link
Author

h4shk4t commented May 21, 2023

Okay will do that

@juntao
Copy link
Member

juntao commented May 22, 2023

flows summarize

Copy link
Member

juntao commented May 22, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


There are potential security issues with the Redis connection in the implementation, and there are no tests or documentation added for the Redis functionality. The purpose of the Redis example is not clear, and the implementation could benefit from improved data structures for the use case. There are also unused dependencies and no tests included. These issues need to be addressed before the pull request can be merged.

Details

Commit 5c745acc26b238a0e6e46a3d07f0c6de44396993

Key changes:

  • An implementation for Redis data source and parser have been added to the mega_etl library.
  • The DataSource enum now includes a Redis variant that takes in a hostname and password for the Redis connection.
  • The match block in the DataSource::from_url method now includes a case for Redis data source.
  • Method Pipe::transform now has an implementation for Redis connections based on the input data source.

Potential problems:

  • The Redis connection URL is derived using string concatenation, which is generally not recommended for security reasons. It would be better to use a URL parsing and concatenation library.
  • There may be security issues if the Redis connection password is hardcoded or provided in plain text.
  • There are no tests or documentation added for the newly implemented Redis functionality.
  • The "password" in DataSource::Redis could mean an empty password, which is valid for an unsecured Redis instance in some cases. However, the current implementation treats "Incorrect Password Provided!" as the password if none is provided, which could cause issues with validation and potentially crashing the program.

Commit db2be299769dee9ef76f7aa9699a773cf4edb6e8

Key Changes:

  • Adds a new directory under examples with a Redis example
  • Provides a Cargo.toml file with dependencies required for the Redis example
  • Defines structs for Order, a sample data structure for the Redis example
  • Implements async_trait and the Transformer trait for processing the order, and checks for errors
  • Implements transform method that transforms inbound data and pushes the data to output
  • Implements init method which initializes the transformer
  • Implements the main method that starts the pipeline

Potential Problems:

  • The purpose of the implementation is not clearly defined, it's unclear what this code is responsible for.
  • The Order data structure does not look production-ready and should be replaced with something more realistic for the use-case.
  • It is unclear why a Redis example was added to Mega ETL and if it is necessary.
  • The Db struct is not shared in this pull request, so it's unclear how it's used.
  • The format function of the Order struct doesn't appear to be doing anything useful.
  • There are a number of unused dependencies and t here are also no tests included.

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