DB integration #781

aurangzaib048 · 2024-03-05T15:35:48Z

Resolves: #762

porteron

Overall it looks clean. I've never used alembic before but it looks pretty straightforward. The migrations setup looks like it will develop nicely over time. I don't see any queries or migrations to create the database user(s). Maybe we should include that.

db/alembic/env.py

porteron

Looks good

github-actions · 2024-03-15T22:55:31Z

db/alembic/versions/2024_03_05_1334-ec0e245c0a9e_initialize_schema_and_pk_generation.py

+                end;
+                $$ language plpgsql;
+            """  # nosec
+        )


_{reported by reviewdog 🐶}
[semgrep] sqlalchemy.text passes the constructed SQL statement to the database mostly unchanged. This means that the usual SQL injection protections are not applied and this function is vulnerable to SQL injection if user input can reach here. Use normal SQLAlchemy operators (such as or_, and_, etc.) to construct SQL.

Source: https://semgrep.dev/r/python.sqlalchemy.security.audit.avoid-sqlalchemy-text.avoid-sqlalchemy-text

Cc @thypon @bcaller

db/alembic/versions/2024_03_05_1334-ec0e245c0a9e_initialize_schema_and_pk_generation.py

porteron

Looks good. Not sure if it exists somewhere already but it would be great to add to the README some generic information about the schema design, how and why we are using alembic, how migrations are managed.

porteron · 2024-03-30T00:09:33Z

db/README.md

Thank you 🙏

Super helpful

github-actions · 2024-04-01T14:34:14Z

[puLL-Merge] - brave/news-aggregator@781

Here is my review of the PR:

Description

This PR makes several major changes to integrate a PostgreSQL database using SQLAlchemy ORM to store publishers, feeds, articles, and related data. It adds database migrations using Alembic. The aggregator logic is updated to check the database for existing processed articles before re-processing them. Articles are inserted into the database after aggregation.

Changes

Changes

README.md - Updated documentation on project structure and added a link to detailed DB documentation
alembic.ini - Added configuration for Alembic database migration tool
bin/start.sh - Shell script updated to run DB migrations and insert publishers on startup
config.py - Added database connection config and helper to create a DB session
db/ - New directory containing:
- README.md - Detailed documentation on DB schema design, Alembic usage and migration management
- alembic/ - Contains Alembic migration scripts
- setup/init.sql - SQL script to initialize news schema before running migrations
- tables/ - Contains SQLAlchemy ORM definitions for tables
requirements.txt - Added new dependencies: alembic, psycopg2, sqlalchemy
src/aggregator/aggregate.py - Updated to return processed article data from DB
src/aggregator/processor.py - Checks database for existing processed articles
src/csv_to_global_json.py - Minor logging statement added
src/db_crud.py - New file containing database CRUD operations
src/main.py - Inserts aggregated articles into the database
tests/unit/aggregator/test_processor.py - Updated unit test

Security Hotspots

High - db/alembic/env.py contains raw SQL execution to initialize schema. This should be reviewed to prevent SQL injection if any dynamic values are used. Consider using parameterization.
Medium - src/db_crud.py has several database queries constructed using string formatting. While the current inputs look safe, using SQL parameters would be more secure to prevent injection if any of these become dynamic in future.
Low - The database connection URL is read from config. Ensure this config value is not logged or exposed in error messages, as it may contain sensitive credentials.
Low - Adding a database increases the attack surface. Follow security best practices like using a least-privileged DB user, encrypting data at rest if needed, and keeping dependencies up-to-date.

Let me know if you would like me to elaborate on any part of the review!

aurangzaib048 · 2024-04-01T14:34:14Z

Infra PR: https://github.com/brave-intl/today-ops/pull/154, only enabled for US for now.

DB initial migration

f5630bf

aurangzaib048 changed the title ~~DB initial migration~~ DB integration Mar 5, 2024

Added more migrations/DDLs

4b3855a

aurangzaib048 requested a review from porteron March 6, 2024 13:38

porteron previously approved these changes Mar 6, 2024

View reviewed changes

aurangzaib048 requested a review from aekeus March 6, 2024 20:28

Added tables entities

c2e941c

aurangzaib048 dismissed porteron’s stale review via c2e941c March 7, 2024 14:18

github-actions bot added the puLL-Merge label Mar 7, 2024

Added user for permission

144869b

github-actions bot reviewed Mar 8, 2024

View reviewed changes

db/alembic/env.py Show resolved Hide resolved

github-actions bot added the needs-security-review label Mar 8, 2024

github-actions bot assigned bcaller and thypon Mar 8, 2024

porteron previously approved these changes Mar 11, 2024

View reviewed changes

Alembic doesn't support User creation and grants

686b706

aurangzaib048 dismissed porteron’s stale review via 686b706 March 12, 2024 17:44

aurangzaib048 added 5 commits March 12, 2024 23:03

Create parameterize query where possible

2f21f98

Fixed config.py

57556ca

Fixed indexes in entity models

6c2cfc1

Rename fields

684d643

Fix migrations and added inserts for publishers

ca3381a

github-actions bot reviewed Mar 15, 2024

View reviewed changes

aurangzaib048 added 7 commits March 18, 2024 23:43

Add to_insert functions

ee83ca9

Add insertion.py

bde8c51

initial insertion.py completed

d59c8a5

Fixed migrations

567c976

Add relationships

f23f25e

Fix insertion

2899052

Complete the fetching for publisher which has only one feed

ccc9b7b

aurangzaib048 added 3 commits March 23, 2024 02:54

Complete CRUD for publishers

31d44e4

Add article insertion

2299c11

Fix the migration and add helper functions

7552911

aurangzaib048 requested a review from tarikdem March 28, 2024 15:01

Fix tests and improve db functions

d1a55a7

porteron previously approved these changes Mar 28, 2024

View reviewed changes

Add README.md for database

cea8dac

aurangzaib048 dismissed porteron’s stale review via cea8dac March 29, 2024 17:40

Apply migrations before starting

0a621bb

porteron reviewed Mar 30, 2024

View reviewed changes

porteron previously approved these changes Mar 30, 2024

View reviewed changes

Update the main README.md

86dac1a

aurangzaib048 dismissed porteron’s stale review via 86dac1a April 1, 2024 14:33

aurangzaib048 merged commit b4ca7f0 into master Apr 2, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DB integration #781

DB integration #781

aurangzaib048 commented Mar 5, 2024 •

edited

porteron left a comment

porteron left a comment

github-actions bot Mar 15, 2024

porteron left a comment

porteron Mar 30, 2024

porteron Mar 30, 2024

github-actions bot commented Apr 1, 2024

Changes

aurangzaib048 commented Apr 1, 2024 •

edited

DB integration #781

DB integration #781

Conversation

aurangzaib048 commented Mar 5, 2024 • edited

porteron left a comment

Choose a reason for hiding this comment

porteron left a comment

Choose a reason for hiding this comment

github-actions bot Mar 15, 2024

Choose a reason for hiding this comment

porteron left a comment

Choose a reason for hiding this comment

porteron Mar 30, 2024

Choose a reason for hiding this comment

porteron Mar 30, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 1, 2024

Description

Changes

Security Hotspots

aurangzaib048 commented Apr 1, 2024 • edited

aurangzaib048 commented Mar 5, 2024 •

edited

aurangzaib048 commented Apr 1, 2024 •

edited