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

feat(oonifindings): add postgresql support #862

Merged
merged 11 commits into from
Aug 10, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -84,4 +84,4 @@ def upgrade() -> None:

def downgrade() -> None:
op.drop_table("oonirun_nettest")
op.drop_table("oonirun")
op.drop_table("oonirun")
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
"""init oonifindings

Revision ID: a037e908f3a0
Revises: c9119c05cf42
Create Date: 2024-07-17 16:45:25.752551

"""
from typing import Sequence, Union

from alembic import op
import sqlalchemy as sa


# revision identifiers, used by Alembic.
revision: str = 'a037e908f3a0'
down_revision: Union[str, None] = "c9119c05cf42"
branch_labels: Union[str, Sequence[str], None] = None
depends_on: Union[str, Sequence[str], None] = None


def upgrade() -> None:
op.create_table(
"oonifinding",
sa.Column(
"incident_id",
sa.String(),
nullable=False,
primary_key=True,
),
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a column which is called something like finding_slug?

Also since we are now calling them findings, maybe this should be called finding_id or maybe even just id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you elaborate a little more on what the finding_slug column represents? made the finding_id change here: 2289ced

Copy link
Member

Choose a reason for hiding this comment

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

Basically we would like to have the ability to have some kind of slug for each finding so that instead of it being this opaque number for example https://explorer.ooni.org/findings/45013413801 it should be possible to configure it be https://explorer.ooni.org/findings/2024-07-bangladesh-blocks-facebook. This needs to be stored in the DB and we need to have the ability to lookup both by number (we can assume a number is the old format) and then to a permanent redirect to the new slug based format (which is nicer for humans and search engines).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for explaining this! I added a nullable finding_slug column for now: https://github.com/ooni/backend/pull/862/files#diff-ebc4627fea3725c4e6f1dafa16bfb842ed5cfdb74c8b292e203fc07cd2581ecbR30 along with a TODO note in the models. We should be able to add the endpoint in the next version and also make this non-nullable (if required)

sa.Column("create_time", sa.DateTime(timezone=True), nullable=False),
sa.Column("update_time", sa.DateTime(timezone=True), nullable=False),
sa.Column("start_time", sa.DateTime(timezone=True), nullable=True),
sa.Column("end_time", sa.DateTime(timezone=True), nullable=True),
sa.Column("creator_account_id", sa.String(), nullable=False),
sa.Column("title", sa.String(), nullable=False),
sa.Column("short_description", sa.String(), nullable=False),
sa.Column("text", sa.String(), nullable=False),
sa.Column("reported_by", sa.String(), nullable=False),
sa.Column("email_address", sa.String(), nullable=False),
sa.Column("event_type", sa.String(), nullable=False),
sa.Column("published", sa.Integer(), nullable=False),
sa.Column("deleted", sa.Integer(), nullable=False, default=0),
sa.Column("CCs", sa.JSON(), nullable=True),
sa.Column("ASNs", sa.JSON(), nullable=True),
Copy link
Member

Choose a reason for hiding this comment

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

Can we use consistent capitalization for these columns?

I would suggest country_codes and asns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done here: af0efde

sa.Column("domains", sa.JSON(), nullable=True),
sa.Column("tags", sa.JSON(), nullable=True),
sa.Column("links", sa.JSON(), nullable=True),
sa.Column("test_names", sa.JSON(), nullable=True),
)


def downgrade() -> None:
op.drop_table("oonifinding")
1 change: 0 additions & 1 deletion ooniapi/common/src/common/models.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
from datetime import datetime, timezone
from typing import List, Dict, Any
from sqlalchemy.types import DateTime, TypeDecorator


Expand Down

This file was deleted.

15 changes: 5 additions & 10 deletions ooniapi/services/oonifindings/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ dependencies = [
"statsd ~= 4.0.1",
"uvicorn ~= 0.25.0",
"httpx ~= 0.26.0",
"psycopg2 ~= 2.9.9",
"pyjwt ~= 2.8.0",
"alembic ~= 1.13.1",
"prometheus-fastapi-instrumentator ~= 6.1.0",
"prometheus-client",
]
Expand Down Expand Up @@ -55,17 +57,14 @@ include = ["BUILD_LABEL"]
packages = ["src/oonifindings"]
artifacts = ["BUILD_LABEL"]

[tool.hatch.metadata]
allow-direct-references = true

[tool.hatch.envs.default]
dependencies = [
"pytest",
"pytest-cov",
"click",
"black",
"pytest-asyncio",
"pytest-docker",
"pytest-postgresql",
]
path = ".venv/"

Expand All @@ -78,12 +77,8 @@ cov = ["test-cov", "cov-report"]
[[tool.hatch.envs.all.matrix]]
python = ["3.8", "3.9", "3.10", "3.11", "3.12"]

[tool.hatch.envs.types]
dependencies = [
"mypy>=1.0.0",
]
[tool.hatch.envs.types.scripts]
check = "mypy --install-types --non-interactive {args:src/oonifindings tests}"
[tool.pytest.ini_options]
addopts = ["--import-mode=importlib"]

[tool.coverage.run]
source_pkgs = ["oonifindings", "tests"]
Expand Down
15 changes: 10 additions & 5 deletions ooniapi/services/oonifindings/src/oonifindings/dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,19 @@

from fastapi import Depends

from clickhouse_driver import Client as Clickhouse
from sqlalchemy import create_engine
from sqlalchemy.orm import sessionmaker

from .common.config import Settings
from .common.dependencies import get_settings

def get_clickhouse_session(settings: Annotated[Settings, Depends(get_settings)]):
db = Clickhouse.from_url(settings.clickhouse_url)

def get_postgresql_session(settings: Annotated[Settings, Depends(get_settings)]):
engine = create_engine(settings.postgresql_url)
SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine)

db = SessionLocal()
try:
yield db
finally:
db.disconnect()
finally:
db.close()
20 changes: 7 additions & 13 deletions ooniapi/services/oonifindings/src/oonifindings/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,13 @@

from prometheus_fastapi_instrumentator import Instrumentator

from . import models
from .routers import v1

from .dependencies import get_settings, get_clickhouse_session
from .dependencies import get_settings, get_postgresql_session
from .common.version import get_build_label, get_pkg_version
from .common.clickhouse_utils import query_click
from .common.metrics import mount_metrics


pkg_name = "oonifindings"

pkg_version = get_pkg_version(pkg_name)
Expand Down Expand Up @@ -67,17 +66,12 @@ class HealthStatus(BaseModel):
@app.get("/health")
async def health(
settings=Depends(get_settings),
db=Depends(get_clickhouse_session),
db=Depends(get_postgresql_session),
):
errors = []

try:
query = f"""SELECT id, update_time, start_time, end_time, reported_by,
title, event_type, published, CCs, ASNs, domains, tags, test_names,
links, short_description, email_address, create_time, creator_account_id
FROM incidents FINAL
"""
query_click(db=db, query=query, query_params={})
db.query(models.OONIFinding).limit(1).all()
except Exception as exc:
log.error(exc)
errors.append("db_error")
Expand All @@ -86,17 +80,17 @@ async def health(
err = "bad_jwt_secret"
log.error(err)
errors.append(err)

if settings.prometheus_metrics_password == "CHANGEME":
err = "bad_prometheus_password"
log.error(err)
errors.append(err)

if len(errors) > 0:
raise HTTPException(status_code=400, detail="health check failed")

status = "ok"

return {
"status": status,
"errors": errors,
Expand Down
37 changes: 37 additions & 0 deletions ooniapi/services/oonifindings/src/oonifindings/models.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
from datetime import datetime
from typing import List
from sqlalchemy import String
from sqlalchemy.orm import Mapped
from sqlalchemy.orm import mapped_column

from .common.models import UtcDateTime
from .common.postgresql import Base


class OONIFinding(Base):
__tablename__ = "oonifinding"

# TODO(decfox): add primary key finding id
incident_id: Mapped[str] = mapped_column(String, primary_key=True)

create_time: Mapped[datetime] = mapped_column(UtcDateTime())
update_time: Mapped[datetime] = mapped_column(UtcDateTime())
start_time: Mapped[datetime] = mapped_column(UtcDateTime(), nullable=True)
end_time: Mapped[datetime] = mapped_column(UtcDateTime(), nullable=True)
creator_account_id: Mapped[str] = mapped_column(String(32))

title: Mapped[str] = mapped_column()
short_description: Mapped[str] = mapped_column()
text: Mapped[str] = mapped_column()
reported_by: Mapped[str] = mapped_column()
email_address: Mapped[str] = mapped_column()
event_type: Mapped[str] = mapped_column()
published: Mapped[int] = mapped_column()
deleted: Mapped[int] = mapped_column(default=0)

CCs: Mapped[List[str]] = mapped_column(nullable=True)
ASNs: Mapped[List[str]] = mapped_column(nullable=True)
domains: Mapped[List[str]] = mapped_column(nullable=True)
tags: Mapped[List[str]] = mapped_column(nullable=True)
links: Mapped[List[str]] = mapped_column(nullable=True)
test_names: Mapped[List[str]] = mapped_column(nullable=True)
Loading
Loading