From e2b9c023a60dbde95ecae8675ccaf2b7127d863d Mon Sep 17 00:00:00 2001 From: Ochoa-Stack <195959137+Ochoa-Stack@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:53:10 -0600 Subject: [PATCH 1/7] test(panorama): add query-count characterization test for get_compare MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Documenta el comportamiento N+1 actual de get_compare (confirmado: 15 queries exactas para 5 skills, 3 por skill). Este test se actualiza, no se elimina, cuando la Ronda 3 introduzca los métodos batch que reduzcan el conteo a 3 queries. Co-authored-by: Oscar Soriano Co-authored-by: Aylin Chavira Co-authored-by: Alejandro Balderrama --- .../integration/test_panorama_endpoints.py | 62 +++++++++++-------- 1 file changed, 35 insertions(+), 27 deletions(-) diff --git a/backend/tests/integration/test_panorama_endpoints.py b/backend/tests/integration/test_panorama_endpoints.py index d4b7306..3e2798e 100644 --- a/backend/tests/integration/test_panorama_endpoints.py +++ b/backend/tests/integration/test_panorama_endpoints.py @@ -1,5 +1,6 @@ import uuid import pytest +from sqlalchemy import event from datetime import datetime, timezone, timedelta from app.models.city import City @@ -10,6 +11,14 @@ from app.models.trend_snapshot import TrendSnapshot from app.extensions import db as _db +@pytest.fixture +def query_counter(app): + counts = {"n": 0} + def on_execute(conn, cursor, statement, parameters, context, executemany): + counts["n"] += 1 + event.listen(_db.engine, "before_cursor_execute", on_execute) + yield counts + event.remove(_db.engine, "before_cursor_execute", on_execute) # Helpers de creacion de entidades. # Duplicados deliberadamente aqui (principio DAMP): cada archivo de tests es autocontenido. No se importan desde otros archivos de tests. @@ -21,21 +30,18 @@ def _make_city(db_session, city_id=2, name="Ciudad Test", state="Estado Test"): db_session.flush() return city - def _make_category(db_session, name="Programacion"): cat = Category(name=name) db_session.add(cat) db_session.flush() return cat - def _make_skill(db_session, category_id, name="Python"): skill = Skill(name=name, canonical_name=name.lower(), category_id=category_id) db_session.add(skill) db_session.flush() return skill - def _make_job(db_session, city_id, salary_min=None, salary_max=None): job = Job( source="test", @@ -53,7 +59,6 @@ def _make_job(db_session, city_id, salary_min=None, salary_max=None): db_session.flush() return job - def _make_job_skill(db_session, job_id, skill_id, confidence_score=0.9): js = JobSkill( job_id=job_id, @@ -64,7 +69,6 @@ def _make_job_skill(db_session, job_id, skill_id, confidence_score=0.9): db_session.flush() return js - def _make_snapshot(db_session, skill_id, city_id, demand_count=10, days_ago=0, growth_rate=None, avg_salary=None): """ Inserta un TrendSnapshot directamente (sin pasar por generate_snapshots). days_ago=0 => fecha de hoy; days_ago=7 => hace 7 dias. """ @@ -81,6 +85,15 @@ def _make_snapshot(db_session, skill_id, city_id, demand_count=10, db_session.commit() return snap +def _setup_compare_skills(db_session, num_skills=2, base_id=40): + city = _make_city(db_session, city_id=base_id, name=f"City_Compare_{base_id}") + cat = _make_category(db_session, name=f"Cat_Compare_{base_id}") + skill_ids = [] + for i in range(num_skills): + skill = _make_skill(db_session, cat.id, name=f"Skill_Compare_{base_id}_{i}") + _make_snapshot(db_session, skill.id, city.id, demand_count=(i+1)*5) + skill_ids.append(skill.id) + return skill_ids # GET /api/panorama/skills @@ -129,7 +142,6 @@ def test_get_catalogs_returns_skills_and_cities(app, db_session, client): any_city = next(c for c in data["cities"] if c["id"] == city_id) assert "name" in any_city - # GET /api/panorama/summary def test_get_summary_returns_kpis(app, db_session, client): @@ -151,7 +163,6 @@ def test_get_summary_returns_kpis(app, db_session, client): assert data["total_jobs"] >= 1 assert data["total_skills_tracked"] >= 1 - # GET /api/panorama/skills/top def test_get_skills_top_respects_limit_bounds(app, client): @@ -166,7 +177,6 @@ def test_get_skills_top_respects_limit_bounds(app, client): assert resp_over.status_code == 200 assert isinstance(resp_over.get_json()["data"], list) - # GET /api/panorama/trends def test_get_trends_requires_skill_id(app, client): @@ -176,7 +186,6 @@ def test_get_trends_requires_skill_id(app, client): assert response.status_code == 422 assert response.get_json()["error"]["code"] == "VALIDATION_ERROR" - def test_get_trends_returns_404_for_nonexistent_skill(app, client): """ GET con skill_id inexistente debe retornar 404 NOT_FOUND. """ response = client.get("/api/panorama/trends?skill_id=999999") @@ -184,7 +193,6 @@ def test_get_trends_returns_404_for_nonexistent_skill(app, client): assert response.status_code == 404 assert response.get_json()["error"]["code"] == "NOT_FOUND" - def test_get_trends_returns_series_for_valid_skill(app, db_session, client): """ Crear skill con un trend_snapshot. Verificar 200 y que la serie temporal incluye la fecha del snapshot insertado. """ city = _make_city(db_session, city_id=30, name="City_Trends") @@ -204,7 +212,6 @@ def test_get_trends_returns_series_for_valid_skill(app, db_session, client): series_dates = [s["date"] for s in data["series"]] assert str(snap_date) in series_dates - # GET /api/panorama/geo def test_get_geo_rejects_invalid_group_by(app, client): @@ -214,7 +221,6 @@ def test_get_geo_rejects_invalid_group_by(app, client): assert response.status_code == 422 assert response.get_json()["error"]["code"] == "VALIDATION_ERROR" - def test_get_geo_returns_404_for_nonexistent_skill(app, client): """ skill_id inexistente debe retornar 404 NOT_FOUND. """ response = client.get("/api/panorama/geo?skill_id=999999") @@ -222,7 +228,6 @@ def test_get_geo_returns_404_for_nonexistent_skill(app, client): assert response.status_code == 404 assert response.get_json()["error"]["code"] == "NOT_FOUND" - def test_get_geo_returns_200_without_skill_filter(app, client): """ Sin skill_id el endpoint debe retornar 200 con distribucion global (puede estar vacia si no hay snapshots, pero no debe fallar). """ response = client.get("/api/panorama/geo") @@ -232,7 +237,6 @@ def test_get_geo_returns_200_without_skill_filter(app, client): assert "distribution" in data assert isinstance(data["distribution"], list) - # GET /api/panorama/salaries def test_get_salaries_requires_skill_id(app, client): @@ -242,7 +246,6 @@ def test_get_salaries_requires_skill_id(app, client): assert response.status_code == 422 assert response.get_json()["error"]["code"] == "VALIDATION_ERROR" - def test_get_salaries_returns_404_for_nonexistent_skill(app, client): """ skill_id inexistente debe retornar 404 NOT_FOUND. """ response = client.get("/api/panorama/salaries?skill_id=999999") @@ -250,7 +253,6 @@ def test_get_salaries_returns_404_for_nonexistent_skill(app, client): assert response.status_code == 404 assert response.get_json()["error"]["code"] == "NOT_FOUND" - # GET /api/panorama/compare def test_get_compare_requires_between_2_and_5_skills(app, db_session, client): @@ -270,7 +272,6 @@ def test_get_compare_requires_between_2_and_5_skills(app, db_session, client): assert resp_six.status_code == 422 assert resp_six.get_json()["error"]["code"] == "VALIDATION_ERROR" - def test_get_compare_returns_404_when_any_skill_missing(app, db_session, client): """ Un skill real + un id inexistente debe retornar 404 NOT_FOUND. """ cat = _make_category(db_session, name="Cat_Compare_404") @@ -282,17 +283,10 @@ def test_get_compare_returns_404_when_any_skill_missing(app, db_session, client) assert response.status_code == 404 assert response.get_json()["error"]["code"] == "NOT_FOUND" - def test_get_compare_success_with_valid_skills(app, db_session, client): """ Crear dos skills con al menos un snapshot cada uno. Verificar 200 y que la respuesta incluye un bloque por cada skill solicitado. """ - city = _make_city(db_session, city_id=40, name="City_Compare") - cat = _make_category(db_session, name="Cat_Compare_OK") - skill_a = _make_skill(db_session, cat.id, name="Skill_Compare_A") - skill_b = _make_skill(db_session, cat.id, name="Skill_Compare_B") - _make_snapshot(db_session, skill_a.id, city.id, demand_count=8) - _make_snapshot(db_session, skill_b.id, city.id, demand_count=12) - id_a = skill_a.id - id_b = skill_b.id + skill_ids = _setup_compare_skills(db_session, num_skills=2, base_id=40) + id_a, id_b = skill_ids response = client.get(f"/api/panorama/compare?skill_ids={id_a},{id_b}") @@ -302,4 +296,18 @@ def test_get_compare_success_with_valid_skills(app, db_session, client): returned_skill_ids = {block["skill_id"] for block in data["skills"]} assert id_a in returned_skill_ids - assert id_b in returned_skill_ids \ No newline at end of file + assert id_b in returned_skill_ids + +def test_get_compare_query_count_baseline_before_optimization( + client, db_session, query_counter +): + """ Test de caracterización: documenta el número EXACTO de queries que get_compare ejecuta hoy con 5 skills (patrón N+1 confirmado en auditoría: hasta 15 queries). Este test debe actualizarse, no eliminarse, cuando la Ronda 3 introduzca los métodos batch. """ + skill_ids = _setup_compare_skills(db_session, num_skills=5, base_id=50) + ids_str = ",".join(str(i) for i in skill_ids) + + query_counter["n"] = 0 + response = client.get(f"/api/panorama/compare?skill_ids={ids_str}") + + assert response.status_code == 200 + + assert query_counter["n"] == 15 From 74b110463323ab7d89d443d93142fdea2beaeb45 Mon Sep 17 00:00:00 2001 From: Ochoa-Stack <195959137+Ochoa-Stack@users.noreply.github.com> Date: Fri, 31 Jul 2026 17:02:43 -0600 Subject: [PATCH 2/7] feat(repositories): add batch query methods for skills and trend snapshots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Agrega SkillRepository.get_by_ids, TrendSnapshotRepository.get_by_skill_ids y TrendSnapshotRepository.get_latest_by_skill_ids (via DISTINCT ON) para resolver DT-26. Sin consumidores todavía; panorama_bp.get_compare se migra en la siguiente ronda. Co-authored-by: Oscar Soriano Co-authored-by: Aylin Chavira Co-authored-by: Alejandro Balderrama --- backend/app/repositories/skill_repository.py | 6 + .../repositories/trend_snapshot_repository.py | 18 +++ .../unit/test_repository_batch_methods.py | 103 ++++++++++++++++++ 3 files changed, 127 insertions(+) create mode 100644 backend/tests/unit/test_repository_batch_methods.py diff --git a/backend/app/repositories/skill_repository.py b/backend/app/repositories/skill_repository.py index cdeb1a4..c4a0303 100644 --- a/backend/app/repositories/skill_repository.py +++ b/backend/app/repositories/skill_repository.py @@ -19,6 +19,12 @@ def get_by_canonical_name(cls, canonical_name: str) -> Skill: db.select(Skill).filter_by(canonical_name=canonical_name) ).scalar_one_or_none() + @classmethod + def get_by_ids(cls, skill_ids: list[int]) -> list: + return db.session.execute( + db.select(Skill).filter(Skill.id.in_(skill_ids)) + ).scalars().all() + @classmethod def get_salary_stats(cls, skill_id: int): from sqlalchemy import func diff --git a/backend/app/repositories/trend_snapshot_repository.py b/backend/app/repositories/trend_snapshot_repository.py index da65d42..a5a170f 100644 --- a/backend/app/repositories/trend_snapshot_repository.py +++ b/backend/app/repositories/trend_snapshot_repository.py @@ -16,6 +16,16 @@ def get_latest_by_skill(cls, skill_id: int): .limit(1) ).scalar_one_or_none() + @classmethod + def get_latest_by_skill_ids(cls, skill_ids: list[int]) -> list: + # DISTINCT ON requiere que el primer campo de ORDER BY coincida con la columna de distinct, así garantizamos una fila por skill_id: la de fecha mas reciente (date DESC). + return db.session.execute( + db.select(TrendSnapshot) + .filter(TrendSnapshot.skill_id.in_(skill_ids)) + .distinct(TrendSnapshot.skill_id) + .order_by(TrendSnapshot.skill_id, desc(TrendSnapshot.date)) + ).scalars().all() + @classmethod def get_by_skill_city_date(cls, skill_id: int, city_id: int, target_date): return db.session.execute( @@ -66,6 +76,14 @@ def get_by_skill_id(cls, skill_id: int) -> list: .order_by(TrendSnapshot.date) ).scalars().all() + @classmethod + def get_by_skill_ids(cls, skill_ids: list[int]) -> list: + return db.session.execute( + db.select(TrendSnapshot) + .filter(TrendSnapshot.skill_id.in_(skill_ids)) + .order_by(TrendSnapshot.skill_id, TrendSnapshot.date) + ).scalars().all() + @classmethod def get_by_city_id(cls, city_id: int) -> list: return db.session.execute( diff --git a/backend/tests/unit/test_repository_batch_methods.py b/backend/tests/unit/test_repository_batch_methods.py new file mode 100644 index 0000000..68cf736 --- /dev/null +++ b/backend/tests/unit/test_repository_batch_methods.py @@ -0,0 +1,103 @@ +import pytest +from datetime import datetime, timezone, timedelta + +from app.models.city import City +from app.models.category import Category +from app.models.skill import Skill +from app.models.trend_snapshot import TrendSnapshot +from app.repositories.skill_repository import SkillRepository +from app.repositories.trend_snapshot_repository import TrendSnapshotRepository + +# Helpers +def _make_city(db_session, city_id=2, name="City"): + city = City(id=city_id, name=name, state="State", country="MX") + db_session.add(city) + db_session.flush() + return city + +def _make_category(db_session, name="Category"): + cat = Category(name=name) + db_session.add(cat) + db_session.flush() + return cat + +def _make_skill(db_session, category_id, name="Skill"): + skill = Skill(name=name, canonical_name=name.upper(), category_id=category_id) + db_session.add(skill) + db_session.flush() + return skill + +def _make_snapshot(db_session, skill_id, city_id, demand_count=10, days_ago=0): + snap_date = datetime.now(timezone.utc).date() - timedelta(days=days_ago) + snap = TrendSnapshot( + skill_id=skill_id, + city_id=city_id, + date=snap_date, + demand_count=demand_count, + growth_rate=None, + avg_salary=None, + ) + db_session.add(snap) + db_session.commit() + return snap + +def test_skill_get_by_ids_returns_matching_skills_only(app, db_session): + cat = _make_category(db_session) + s1 = _make_skill(db_session, cat.id, name="S1") + s2 = _make_skill(db_session, cat.id, name="S2") + s3 = _make_skill(db_session, cat.id, name="S3") + + with app.app_context(): + skills = SkillRepository.get_by_ids([s1.id, s3.id]) + assert len(skills) == 2 + ids = {s.id for s in skills} + assert s1.id in ids + assert s3.id in ids + assert s2.id not in ids + +def test_trend_snapshot_get_by_skill_ids_returns_all_snapshots_for_given_skills(app, db_session): + cat = _make_category(db_session) + city = _make_city(db_session) + s1 = _make_skill(db_session, cat.id, name="SnapS1") + s2 = _make_skill(db_session, cat.id, name="SnapS2") + + _make_snapshot(db_session, s1.id, city.id, days_ago=1) + _make_snapshot(db_session, s1.id, city.id, days_ago=2) + _make_snapshot(db_session, s2.id, city.id, days_ago=3) + _make_snapshot(db_session, s2.id, city.id, days_ago=4) + + with app.app_context(): + snaps = TrendSnapshotRepository.get_by_skill_ids([s1.id, s2.id]) + assert len(snaps) == 4 + +def test_trend_snapshot_get_latest_by_skill_ids_returns_one_per_skill(app, db_session): + cat = _make_category(db_session) + city = _make_city(db_session, city_id=99) + s1 = _make_skill(db_session, cat.id, name="LatestS1") + s2 = _make_skill(db_session, cat.id, name="LatestS2") + + # 3 snapshots per skill, days_ago ensures different dates (1 is most recent) + s1_snap1 = _make_snapshot(db_session, s1.id, city.id, days_ago=1) + s1_snap2 = _make_snapshot(db_session, s1.id, city.id, days_ago=2) + s1_snap3 = _make_snapshot(db_session, s1.id, city.id, days_ago=3) + + s2_snap1 = _make_snapshot(db_session, s2.id, city.id, days_ago=5) + s2_snap2 = _make_snapshot(db_session, s2.id, city.id, days_ago=10) + s2_snap3 = _make_snapshot(db_session, s2.id, city.id, days_ago=15) + + with app.app_context(): + snaps = TrendSnapshotRepository.get_latest_by_skill_ids([s1.id, s2.id]) + assert len(snaps) == 2 + + # Verify exactly one snapshot per skill id and it's the most recent one + s1_returned = next(s for s in snaps if s.skill_id == s1.id) + s2_returned = next(s for s in snaps if s.skill_id == s2.id) + + assert s1_returned.date == s1_snap1.date + assert s2_returned.date == s2_snap1.date + +def test_trend_snapshot_get_latest_by_skill_ids_empty_list_returns_empty(app, db_session): + with app.app_context(): + snaps = TrendSnapshotRepository.get_latest_by_skill_ids([]) + assert isinstance(snaps, list) + assert len(snaps) == 0 From 33d5f9f33c560086b5f52905e9bb0d24eb4bdd14 Mon Sep 17 00:00:00 2001 From: Ochoa-Stack <195959137+Ochoa-Stack@users.noreply.github.com> Date: Fri, 31 Jul 2026 17:12:37 -0600 Subject: [PATCH 3/7] refactor(panorama): resolve DT-26 N+1 in get_compare via PanoramaService Extrae la logica de get_compare a PanoramaService.get_compare_data, usando los metodos batch de la ronda anterior. Reduce el patron N+1 de 15 queries a 3 para 5 habilidades, verificado con test de caracterizacion. Comportamiento observable del endpoint sin cambios. Co-authored-by: Oscar Soriano Co-authored-by: Aylin Chavira Co-authored-by: Alejandro Balderrama --- backend/app/controllers/panorama_bp.py | 32 +++---------- backend/app/services/panorama_service.py | 45 +++++++++++++++++++ .../integration/test_panorama_endpoints.py | 10 ++--- 3 files changed, 55 insertions(+), 32 deletions(-) create mode 100644 backend/app/services/panorama_service.py diff --git a/backend/app/controllers/panorama_bp.py b/backend/app/controllers/panorama_bp.py index b13778b..72e1f3c 100644 --- a/backend/app/controllers/panorama_bp.py +++ b/backend/app/controllers/panorama_bp.py @@ -2,6 +2,7 @@ from app.repositories.skill_repository import SkillRepository from app.repositories.city_repository import CityRepository from app.repositories.trend_snapshot_repository import TrendSnapshotRepository +from app.services.panorama_service import PanoramaService from app.schemas.skill_schema import SkillResponseSchema from app.schemas.panorama_schema import ( CatalogsResponseSchema, @@ -244,39 +245,16 @@ def get_compare(): status_code=422, ) - skills_map = {} - missing_ids = [] - for sid in skill_ids: - skill = SkillRepository.get_by_id(sid) - if skill: - skills_map[sid] = skill - else: - missing_ids.append(sid) + data = PanoramaService.get_compare_data(skill_ids) - if missing_ids: + if data["missing_ids"]: return error_response( code="NOT_FOUND", - message=f"Las siguientes habilidades no existen: {missing_ids}.", + message=f"Las siguientes habilidades no existen: {data['missing_ids']}.", status_code=404, ) - blocks = [] - for sid in skill_ids: - skill = skills_map[sid] - latest = TrendSnapshotRepository.get_latest_by_skill(sid) - series_snapshots = TrendSnapshotRepository.get_by_skill_id(sid) - - blocks.append({ - "skill_id": skill.id, - "skill_name": skill.name, - "demand_count": latest.demand_count if latest else 0, - "growth_rate": latest.growth_rate if latest else None, - "avg_salary": latest.avg_salary if latest else None, - "series": [ - {"date": s.date, "demand_count": s.demand_count} - for s in series_snapshots - ], - }) + blocks = data["blocks"] result = CompareResponseSchema().dump({"skills": blocks}) return success_response(data=result, status_code=200) diff --git a/backend/app/services/panorama_service.py b/backend/app/services/panorama_service.py new file mode 100644 index 0000000..d54ce3f --- /dev/null +++ b/backend/app/services/panorama_service.py @@ -0,0 +1,45 @@ +from app.repositories.skill_repository import SkillRepository +from app.repositories.trend_snapshot_repository import TrendSnapshotRepository + +class PanoramaService: + # Orquesta las consultas necesarias para la vista de comparacion, usando metodos batch para evitar el patron N+1 que antes generaba hasta 15 queries individuales para 5 habilidades. + + @classmethod + def get_compare_data(cls, skill_ids: list[int]) -> dict: + """ + Retorna dict con: + - 'missing_ids': list[int] - ids solicitados que no existen + - 'blocks': list[dict] - un bloque por skill_id válido, en el mismo orden que skill_ids, con la misma forma que el payload que get_compare ya arma hoy (skill_id, skill_name, demand_count, growth_rate, avg_salary, series). Si missing_ids no está vacío, blocks debe ser una lista vacía; el controlador decide si retorna 404, el servicio solo reporta qué falta. """ + skills = SkillRepository.get_by_ids(skill_ids) + skills_by_id = {s.id: s for s in skills} + missing_ids = [sid for sid in skill_ids if sid not in skills_by_id] + + if missing_ids: + return {"missing_ids": missing_ids, "blocks": []} + + latest_snapshots = TrendSnapshotRepository.get_latest_by_skill_ids(skill_ids) + latest_by_skill = {s.skill_id: s for s in latest_snapshots} + + all_snapshots = TrendSnapshotRepository.get_by_skill_ids(skill_ids) + series_by_skill = {} + for snap in all_snapshots: + series_by_skill.setdefault(snap.skill_id, []).append(snap) + + blocks = [] + for sid in skill_ids: + skill = skills_by_id[sid] + latest = latest_by_skill.get(sid) + series = series_by_skill.get(sid, []) + blocks.append({ + "skill_id": skill.id, + "skill_name": skill.name, + "demand_count": latest.demand_count if latest else 0, + "growth_rate": latest.growth_rate if latest else None, + "avg_salary": latest.avg_salary if latest else None, + "series": [ + {"date": s.date, "demand_count": s.demand_count} + for s in series + ], + }) + + return {"missing_ids": [], "blocks": blocks} diff --git a/backend/tests/integration/test_panorama_endpoints.py b/backend/tests/integration/test_panorama_endpoints.py index 3e2798e..b012ec3 100644 --- a/backend/tests/integration/test_panorama_endpoints.py +++ b/backend/tests/integration/test_panorama_endpoints.py @@ -172,7 +172,7 @@ def test_get_skills_top_respects_limit_bounds(app, client): assert resp_zero.status_code == 200 assert isinstance(resp_zero.get_json()["data"], list) - # limit=1000 => max(1, min(1000, 50)) = 50 — no debe fallar + # limit=1000 => max(1, min(1000, 50)) = 50 - no debe fallar resp_over = client.get("/api/panorama/skills/top?limit=1000") assert resp_over.status_code == 200 assert isinstance(resp_over.get_json()["data"], list) @@ -261,12 +261,12 @@ def test_get_compare_requires_between_2_and_5_skills(app, db_session, client): skills = [_make_skill(db_session, cat.id, name=f"Skill_Bound_{i}") for i in range(6)] ids = [s.id for s in skills] - # Un solo skill — menor al minimo de 2 + # Un solo skill - menor al minimo de 2 resp_one = client.get(f"/api/panorama/compare?skill_ids={ids[0]}") assert resp_one.status_code == 422 assert resp_one.get_json()["error"]["code"] == "VALIDATION_ERROR" - # Seis skills — excede el maximo de 5 + # Seis skills - excede el maximo de 5 ids_str = ",".join(str(i) for i in ids) resp_six = client.get(f"/api/panorama/compare?skill_ids={ids_str}") assert resp_six.status_code == 422 @@ -301,7 +301,7 @@ def test_get_compare_success_with_valid_skills(app, db_session, client): def test_get_compare_query_count_baseline_before_optimization( client, db_session, query_counter ): - """ Test de caracterización: documenta el número EXACTO de queries que get_compare ejecuta hoy con 5 skills (patrón N+1 confirmado en auditoría: hasta 15 queries). Este test debe actualizarse, no eliminarse, cuando la Ronda 3 introduzca los métodos batch. """ + """ Test de caracterización: documenta el número EXACTO de queries que get_compare ejecuta hoy con 5 skills. """ skill_ids = _setup_compare_skills(db_session, num_skills=5, base_id=50) ids_str = ",".join(str(i) for i in skill_ids) @@ -310,4 +310,4 @@ def test_get_compare_query_count_baseline_before_optimization( assert response.status_code == 200 - assert query_counter["n"] == 15 + assert query_counter["n"] == 3 From 301b2ca67c96ce81f5ad0e70819a4bb1b0960d07 Mon Sep 17 00:00:00 2001 From: Ochoa-Stack <195959137+Ochoa-Stack@users.noreply.github.com> Date: Fri, 31 Jul 2026 17:30:28 -0600 Subject: [PATCH 4/7] feat(cities): add unaccent-based indexed city name search Habilita la extension unaccent de PostgreSQL (via wrapper immutable_unaccent para permitir indexacion, dado que unaccent nativo es STABLE no IMMUTABLE) y agrega CityRepository.find_by_normalized_name, verificado en 1 query indexada. Prepara la base para DT-27: get_or_create_city y _normalize siguen intactos, se reconectan en la siguiente ronda. Co-authored-by: Oscar Soriano Co-authored-by: Aylin Chavira Co-authored-by: Alejandro Balderrama --- backend/app/repositories/city_repository.py | 14 +++++ ...614f_enable_unaccent_extension_and_add_.py | 34 +++++++++++ .../test_city_repository_unaccent.py | 61 +++++++++++++++++++ 3 files changed, 109 insertions(+) create mode 100644 backend/migrations/versions/e89b2bf6614f_enable_unaccent_extension_and_add_.py create mode 100644 backend/tests/integration/test_city_repository_unaccent.py diff --git a/backend/app/repositories/city_repository.py b/backend/app/repositories/city_repository.py index d6f4de3..b88a403 100644 --- a/backend/app/repositories/city_repository.py +++ b/backend/app/repositories/city_repository.py @@ -19,6 +19,20 @@ def get_by_name(cls, name: str): return db.session.execute( db.select(City).filter_by(name=name) ).scalar_one_or_none() + + @classmethod + def find_by_normalized_name(cls, raw_name: str): + # Usa el indice funcional immutable_unaccent(lower(name)) para busqueda insensible a acentos y mayusculas en una sola query indexada, reemplazando el escaneo completo en memoria que hacia _normalize. + if not raw_name: + return None + raw_name = raw_name.strip() + from sqlalchemy import func + normalized_input = func.immutable_unaccent(func.lower(raw_name)) + return db.session.execute( + db.select(City).filter( + func.immutable_unaccent(func.lower(City.name)) == normalized_input + ) + ).scalar_one_or_none() @classmethod def get_or_create_city(cls, raw_location: str) -> tuple[City | None, bool]: diff --git a/backend/migrations/versions/e89b2bf6614f_enable_unaccent_extension_and_add_.py b/backend/migrations/versions/e89b2bf6614f_enable_unaccent_extension_and_add_.py new file mode 100644 index 0000000..6e0a7c0 --- /dev/null +++ b/backend/migrations/versions/e89b2bf6614f_enable_unaccent_extension_and_add_.py @@ -0,0 +1,34 @@ +"""enable unaccent extension and add functional index on cities name + +Revision ID: e89b2bf6614f +Revises: e269761308d8 +Create Date: 2026-07-31 17:18:04.748824 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'e89b2bf6614f' +down_revision = 'e269761308d8' +branch_labels = None +depends_on = None + + +def upgrade(): + op.execute("CREATE EXTENSION IF NOT EXISTS unaccent;") + op.execute( + "CREATE OR REPLACE FUNCTION immutable_unaccent(text) " + "RETURNS text AS $$ SELECT unaccent('unaccent', $1) $$ " + "LANGUAGE sql IMMUTABLE;" + ) + op.execute( + "CREATE INDEX ix_cities_name_unaccent ON cities " + "(immutable_unaccent(lower(name)));" + ) + +def downgrade(): + op.execute("DROP INDEX IF EXISTS ix_cities_name_unaccent;") + op.execute("DROP FUNCTION IF EXISTS immutable_unaccent(text);") + op.execute("DROP EXTENSION IF EXISTS unaccent;") diff --git a/backend/tests/integration/test_city_repository_unaccent.py b/backend/tests/integration/test_city_repository_unaccent.py new file mode 100644 index 0000000..6d3a908 --- /dev/null +++ b/backend/tests/integration/test_city_repository_unaccent.py @@ -0,0 +1,61 @@ +import pytest +from sqlalchemy import event +from app.extensions import db as _db +from app.models.city import City +from app.repositories.city_repository import CityRepository + +@pytest.fixture +def query_counter(app): + counts = {"n": 0} + def on_execute(conn, cursor, statement, parameters, context, executemany): + counts["n"] += 1 + event.listen(_db.engine, "before_cursor_execute", on_execute) + yield counts + event.remove(_db.engine, "before_cursor_execute", on_execute) + +def _make_city(db_session, name="Guadalajara"): + city = City(name=name, state="State", country="MX") + db_session.add(city) + db_session.flush() + return city + +def test_find_by_normalized_name_matches_exact(app, db_session): + _make_city(db_session, name="Guadalajara") + + with app.app_context(): + city = CityRepository.find_by_normalized_name("Guadalajara") + assert city is not None + assert city.name == "Guadalajara" + +def test_find_by_normalized_name_matches_with_accents_and_case(app, db_session): + _make_city(db_session, name="Querétaro") + + with app.app_context(): + city = CityRepository.find_by_normalized_name(" queretaro ") + assert city is not None + assert city.name == "Querétaro" + +def test_find_by_normalized_name_returns_none_when_not_found(app, db_session): + _make_city(db_session, name="Monterrey") + + with app.app_context(): + city = CityRepository.find_by_normalized_name("Cancun") + assert city is None + +def test_find_by_normalized_name_returns_none_for_empty_string(app, db_session): + with app.app_context(): + city = CityRepository.find_by_normalized_name("") + assert city is None + + city_none = CityRepository.find_by_normalized_name(None) + assert city_none is None + +def test_find_by_normalized_name_uses_single_query(app, db_session, query_counter): + _make_city(db_session, name="Mérida") + + with app.app_context(): + query_counter["n"] = 0 + city = CityRepository.find_by_normalized_name("merida") + assert city is not None + assert city.name == "Mérida" + assert query_counter["n"] == 1 From a9addce92fd08ebc59fc286075cd0841d2f7fc27 Mon Sep 17 00:00:00 2001 From: Ochoa-Stack <195959137+Ochoa-Stack@users.noreply.github.com> Date: Fri, 31 Jul 2026 17:54:40 -0600 Subject: [PATCH 5/7] refactor(cities): move geocoding orchestration to CityService Resuelve DT-27. CityRepository queda reducido a persistencia pura (find_by_normalized_name, get_by_name, create heredado). CityService nuevo orquesta busqueda indexada + Nominatim + persistencia, capturando ConflictError para condiciones de carrera bajo la constraint unique de City.name. ingestion_service.py actualizado para consumir CityService. 11 tests fallan intencionalmente en este punto (esperado, corregidos en la siguiente ronda): 6 en test_city_repository.py probaban el metodo eliminado, 5 en test_ingestion_service.py mockeaban la ruta antigua. Co-authored-by: Oscar Soriano Co-authored-by: Aylin Chavira Co-authored-by: Alejandro Balderrama --- backend/app/repositories/city_repository.py | 51 --------------------- backend/app/services/city_service.py | 37 +++++++++++++++ backend/app/services/ingestion_service.py | 6 +-- 3 files changed, 40 insertions(+), 54 deletions(-) create mode 100644 backend/app/services/city_service.py diff --git a/backend/app/repositories/city_repository.py b/backend/app/repositories/city_repository.py index b88a403..4161818 100644 --- a/backend/app/repositories/city_repository.py +++ b/backend/app/repositories/city_repository.py @@ -1,19 +1,10 @@ from app.repositories.base_repository import BaseRepository from app.models.city import City from app.extensions import db -from app.clients.nominatim_client import NominatimClient -import unicodedata class CityRepository(BaseRepository): model = City - @classmethod - def _normalize(cls, text: str) -> str: - return ''.join( - c for c in unicodedata.normalize('NFD', text.strip().lower()) - if unicodedata.category(c) != 'Mn' - ) - @classmethod def get_by_name(cls, name: str): return db.session.execute( @@ -33,45 +24,3 @@ def find_by_normalized_name(cls, raw_name: str): func.immutable_unaccent(func.lower(City.name)) == normalized_input ) ).scalar_one_or_none() - - @classmethod - def get_or_create_city(cls, raw_location: str) -> tuple[City | None, bool]: - if not raw_location: - return None, False - - # lowercase, sin acentos y trim (Normaliza) - normalized = cls._normalize(raw_location) - - # Búsqueda exhaustiva comparando el nombre normalizado - all_cities = db.session.execute(db.select(City)).scalars().all() - for city in all_cities: - city_norm = cls._normalize(city.name) - if city_norm == normalized: - return city, False - - # Si no existe, llama a geocode_city - geo_data = NominatimClient.geocode_city(raw_location) - if not geo_data: - return None, False - - # Nominatim puede resolver un alias (ej: "Distrito Federal") a un nombre real (ej: "Ciudad de México"). Revisamos si ese nombre real ya existe en BD para evitar IntegrityError secuencial - resolved_name = geo_data["name"] - resolved_norm = cls._normalize(resolved_name) - - for city in all_cities: - city_norm = cls._normalize(city.name) - if city_norm == resolved_norm: - return city, False - - # Si retorna datos válidos y no existe, inserta una nueva fila - new_city = City( - name=geo_data["name"], - state=geo_data["state"], - lat=geo_data["lat"], - lon=geo_data["lon"], - country="MX" - ) - - db.session.add(new_city) - db.session.commit() - return new_city, True diff --git a/backend/app/services/city_service.py b/backend/app/services/city_service.py new file mode 100644 index 0000000..1ad9e4e --- /dev/null +++ b/backend/app/services/city_service.py @@ -0,0 +1,37 @@ +from app.repositories.city_repository import CityRepository +from app.clients.nominatim_client import NominatimClient +from app.utils.errors import ConflictError + +class CityService: + # Orquesta la resolucion de ciudades, va desde la busqueda local indexada, geocodificacion externa via Nominatim, y persistencia. Antes esta orquestacion vivia dentro de CityRepository, violando la separacion de capas de nuestra arquitectura de trabajo (Architecture Hexagonal). + + @classmethod + def get_or_create_city(cls, raw_location: str) -> tuple: + if not raw_location: + return None, False + + existing = CityRepository.find_by_normalized_name(raw_location) + if existing: + return existing, False + + geo_data = NominatimClient.geocode_city(raw_location) + if not geo_data: + return None, False + + # Nominatim puede resolver un alias (ej. "Distrito Federal") a un nombre real (ej. "Ciudad de Mexico") que ya exista en BD. + resolved_name = geo_data["name"] + existing_resolved = CityRepository.find_by_normalized_name(resolved_name) + if existing_resolved: + return existing_resolved, False + + try: + new_city = CityRepository.create({ + "name": geo_data["name"], + "state": geo_data["state"], + "lat": geo_data["lat"], + "lon": geo_data["lon"], + }) + return new_city, True + except ConflictError: + # Condicion de carrera: otro proceso concurrente ya inserto esta misma ciudad entre nuestra verificacion y nuestro intento de creacion. La constraint unique=True de City.name disparo el conflicto; recuperamos la fila que el otro proceso ya persistio, en vez de fallar la ingesta. + return CityRepository.find_by_normalized_name(resolved_name), False diff --git a/backend/app/services/ingestion_service.py b/backend/app/services/ingestion_service.py index 8963bae..f5a7aa9 100644 --- a/backend/app/services/ingestion_service.py +++ b/backend/app/services/ingestion_service.py @@ -8,7 +8,7 @@ from app.repositories.job_repository import JobRepository from app.repositories.skill_repository import SkillRepository from app.repositories.job_skill_repository import JobSkillRepository -from app.repositories.city_repository import CityRepository +from app.services.city_service import CityService from app.utils.errors import AppError logger = logging.getLogger(__name__) @@ -59,7 +59,7 @@ def _process_job(cls, item: dict, known_skills: dict, stats: dict, verbose: bool # Hashing criptográfico para garantizar la idempotencia de la ingesta y evitar guardar la misma vacante si Adzuna la devuelve en días posteriores. desc_hash = hashlib.sha256(description.encode("utf-8")).hexdigest() - # Validar duplicados ANTES de geocodificar o instanciar objetos, para evitar excepciones de BD y transacciones descartadas + # Validamos duplicados ANTES de geocodificar o instanciar objetos, para evitar excepciones de BD y transacciones descartadas if JobRepository.get_by_hash(desc_hash): stats["duplicates"] += 1 return @@ -127,7 +127,7 @@ def _process_job(cls, item: dict, known_skills: dict, stats: dict, verbose: bool @classmethod def _resolve_city(cls, raw_location: str, stats: dict, verbose: bool = False): """Resuelve la ubicación cruda de Adzuna a una fila de la tabla cities. Devuelve (city_id, label_para_log). Usa "México Nacional" como fallback cuando la geocodificación falla o la ubicación está vacía, para garantizar que city_id nunca quede nulo""" - city, created = CityRepository.get_or_create_city(raw_location) if raw_location else (None, False) + city, created = CityService.get_or_create_city(raw_location) if raw_location else (None, False) if city: if created: From a580fb98cf5135b9a0ab46133e5a5eca36042ba3 Mon Sep 17 00:00:00 2001 From: Ochoa-Stack <195959137+Ochoa-Stack@users.noreply.github.com> Date: Fri, 31 Jul 2026 20:15:56 -0600 Subject: [PATCH 6/7] test(cities): relocate get_or_create_city tests to CityService test_city_repository.py reducido a persistencia pura (get_by_name). test_city_service.py nuevo: migra los 6 tests de orquestacion con mocks corregidos al namespace correcto, mas un test nuevo de recuperacion ante ConflictError por condicion de carrera concurrente. test_ingestion_service.py: 5 mocks corregidos de CityRepository a CityService. Suite completa: 140 tests, cero regresiones. Co-authored-by: Oscar Soriano Co-authored-by: Aylin Chavira Co-authored-by: Alejandro Balderrama --- .../tests/integration/test_city_repository.py | 78 ------------ .../tests/integration/test_city_service.py | 116 ++++++++++++++++++ .../integration/test_ingestion_service.py | 10 +- 3 files changed, 121 insertions(+), 83 deletions(-) create mode 100644 backend/tests/integration/test_city_service.py diff --git a/backend/tests/integration/test_city_repository.py b/backend/tests/integration/test_city_repository.py index 4ee38ab..9f90e50 100644 --- a/backend/tests/integration/test_city_repository.py +++ b/backend/tests/integration/test_city_repository.py @@ -10,84 +10,6 @@ def _make_city(db_session, name="Ciudad Test", state="Estado Test", lat=19.4326, db_session.commit() return city -def test_get_or_create_city_returns_existing_exact_match(app, db_session): - """ get_or_create_city retorna una ciudad existente sin crear una nueva cuando el nombre coincide exactamente. """ - _make_city(db_session, name="Guadalajara") - - with app.app_context(): - city, created = CityRepository.get_or_create_city("Guadalajara") - assert not created - assert city is not None - assert city.name == "Guadalajara" - -def test_get_or_create_city_returns_existing_after_normalization(app, db_session): - """ get_or_create_city retorna una ciudad existente cuando el nombre coincide tras normalización (acentos, mayúsculas/minúsculas). """ - _make_city(db_session, name="Querétaro") - - with app.app_context(): - city, created = CityRepository.get_or_create_city(" queretaro ") - assert not created - assert city is not None - assert city.name == "Querétaro" - -def test_get_or_create_city_creates_new_when_valid_data(app, db_session, monkeypatch): - """ get_or_create_city crea una ciudad nueva cuando no existe y Nominatim (mockeado) retorna datos válidos, devolviendo (city, True). """ - mock_geocode = MagicMock(return_value={ - "name": "Monterrey", - "state": "Nuevo León", - "lat": 25.6866, - "lon": -100.3161 - }) - monkeypatch.setattr("app.repositories.city_repository.NominatimClient.geocode_city", mock_geocode) - - with app.app_context(): - city, created = CityRepository.get_or_create_city("Monterrey") - assert created - assert city is not None - assert city.name == "Monterrey" - assert city.state == "Nuevo León" - mock_geocode.assert_called_once_with("Monterrey") - -def test_get_or_create_city_returns_none_when_not_found(app, db_session, monkeypatch): - """ get_or_create_city retorna (None, False) cuando Nominatim (mockeado) no encuentra resultado. """ - mock_geocode = MagicMock(return_value=None) - monkeypatch.setattr("app.repositories.city_repository.NominatimClient.geocode_city", mock_geocode) - - with app.app_context(): - city, created = CityRepository.get_or_create_city("Ciudad Inexistente 123") - assert not created - assert city is None - mock_geocode.assert_called_once_with("Ciudad Inexistente 123") - -def test_get_or_create_city_detects_resolved_name_already_exists(app, db_session, monkeypatch): - """ get_or_create_city detecta que el nombre resuelto por Nominatim ya existe en base de datos aunque el nombre original consultado no coincidiera (caso de alias, ej. "Distrito Federal" resolviendo a "Ciudad de México" ya existente) y retorna esa ciudad sin duplicar. """ - _make_city(db_session, name="Ciudad de México") - - mock_geocode = MagicMock(return_value={ - "name": "Ciudad de México", - "state": "Ciudad de México", - "lat": 19.4326, - "lon": -99.1332 - }) - monkeypatch.setattr("app.repositories.city_repository.NominatimClient.geocode_city", mock_geocode) - - with app.app_context(): - city, created = CityRepository.get_or_create_city("Distrito Federal") - assert not created - assert city is not None - assert city.name == "Ciudad de México" - mock_geocode.assert_called_once_with("Distrito Federal") - -def test_get_or_create_city_returns_none_for_empty_raw_location(app, db_session): - """ get_or_create_city retorna (None, False) cuando raw_location está vacío o es None. """ - with app.app_context(): - city1, created1 = CityRepository.get_or_create_city("") - city2, created2 = CityRepository.get_or_create_city(None) - assert city1 is None - assert not created1 - assert city2 is None - assert not created2 - def test_get_by_name_returns_city_if_exists(app, db_session): """ get_by_name retorna la ciudad correcta cuando existe, y None cuando no. """ _make_city(db_session, name="Cancún") diff --git a/backend/tests/integration/test_city_service.py b/backend/tests/integration/test_city_service.py new file mode 100644 index 0000000..f51a832 --- /dev/null +++ b/backend/tests/integration/test_city_service.py @@ -0,0 +1,116 @@ +import pytest +from unittest.mock import MagicMock + +from app.models.city import City +from app.services.city_service import CityService +from app.repositories.city_repository import CityRepository +from app.utils.errors import ConflictError + +def _make_city(db_session, name="Ciudad Test", state="Estado Test", lat=19.4326, lon=-99.1332): + city = City(name=name, state=state, lat=lat, lon=lon, country="MX") + db_session.add(city) + db_session.commit() + return city + +def test_get_or_create_city_returns_existing_exact_match(app, db_session): + """ get_or_create_city retorna una ciudad existente sin crear una nueva cuando el nombre coincide exactamente. """ + _make_city(db_session, name="Guadalajara") + + with app.app_context(): + city, created = CityService.get_or_create_city("Guadalajara") + assert not created + assert city is not None + assert city.name == "Guadalajara" + +def test_get_or_create_city_returns_existing_after_normalization(app, db_session): + """ get_or_create_city retorna una ciudad existente cuando el nombre coincide tras normalización (acentos, mayúsculas/minúsculas). """ + _make_city(db_session, name="Querétaro") + + with app.app_context(): + city, created = CityService.get_or_create_city(" queretaro ") + assert not created + assert city is not None + assert city.name == "Querétaro" + +def test_get_or_create_city_creates_new_when_valid_data(app, db_session, monkeypatch): + """ get_or_create_city crea una ciudad nueva cuando no existe y Nominatim (mockeado) retorna datos válidos, devolviendo (city, True). """ + mock_geocode = MagicMock(return_value={ + "name": "Monterrey", + "state": "Nuevo León", + "lat": 25.6866, + "lon": -100.3161 + }) + monkeypatch.setattr("app.services.city_service.NominatimClient.geocode_city", mock_geocode) + + with app.app_context(): + city, created = CityService.get_or_create_city("Monterrey") + assert created + assert city is not None + assert city.name == "Monterrey" + assert city.state == "Nuevo León" + mock_geocode.assert_called_once_with("Monterrey") + +def test_get_or_create_city_returns_none_when_not_found(app, db_session, monkeypatch): + """ get_or_create_city retorna (None, False) cuando Nominatim (mockeado) no encuentra resultado. """ + mock_geocode = MagicMock(return_value=None) + monkeypatch.setattr("app.services.city_service.NominatimClient.geocode_city", mock_geocode) + + with app.app_context(): + city, created = CityService.get_or_create_city("Ciudad Inexistente 123") + assert not created + assert city is None + mock_geocode.assert_called_once_with("Ciudad Inexistente 123") + +def test_get_or_create_city_detects_resolved_name_already_exists(app, db_session, monkeypatch): + """ get_or_create_city detecta que el nombre resuelto por Nominatim ya existe en base de datos aunque el nombre original consultado no coincidiera (caso de alias, ej. "Distrito Federal" resolviendo a "Ciudad de México" ya existente) y retorna esa ciudad sin duplicar. """ + _make_city(db_session, name="Ciudad de México") + + mock_geocode = MagicMock(return_value={ + "name": "Ciudad de México", + "state": "Ciudad de México", + "lat": 19.4326, + "lon": -99.1332 + }) + monkeypatch.setattr("app.services.city_service.NominatimClient.geocode_city", mock_geocode) + + with app.app_context(): + city, created = CityService.get_or_create_city("Distrito Federal") + assert not created + assert city is not None + assert city.name == "Ciudad de México" + mock_geocode.assert_called_once_with("Distrito Federal") + +def test_get_or_create_city_returns_none_for_empty_raw_location(app, db_session): + """ get_or_create_city retorna (None, False) cuando raw_location está vacío o es None. """ + with app.app_context(): + city1, created1 = CityService.get_or_create_city("") + city2, created2 = CityService.get_or_create_city(None) + assert city1 is None + assert not created1 + assert city2 is None + assert not created2 + +def test_get_or_create_city_recovers_from_concurrent_creation_conflict(app, db_session, monkeypatch): + """ get_or_create_city recupera la ciudad vía find_by_normalized_name si ocurre un ConflictError al intentar insertarla por una colisión en concurrencia con otro proceso. """ + mock_geocode = MagicMock(return_value={ + "name": "Puebla", + "state": "Puebla", + "lat": 19.0414, + "lon": -98.2063 + }) + monkeypatch.setattr("app.services.city_service.NominatimClient.geocode_city", mock_geocode) + + def mock_create(*args, **kwargs): + # Cuando intenta crearla, simulamos que otro proceso ya la guardo e insertamos directo a BD, + # y luego lanzamos ConflictError para simular el fallo de integridad del proceso actual. + _make_city(db_session, name="Puebla", state="Puebla") + raise ConflictError("Conflicto concurrencia test") + + monkeypatch.setattr("app.services.city_service.CityRepository.create", mock_create) + + with app.app_context(): + city, created = CityService.get_or_create_city("Puebla") + assert not created + assert city is not None + assert city.name == "Puebla" + mock_geocode.assert_called_once_with("Puebla") diff --git a/backend/tests/integration/test_ingestion_service.py b/backend/tests/integration/test_ingestion_service.py index 20058c4..376211a 100644 --- a/backend/tests/integration/test_ingestion_service.py +++ b/backend/tests/integration/test_ingestion_service.py @@ -85,7 +85,7 @@ def test_process_job_extracts_last_area_as_raw_location(monkeypatch): def test_resolve_city_returns_fallback(monkeypatch): """ _resolve_city retorna MEXICO_NACIONAL_CITY_ID y registra stats['fallback'] cuando CityRepository.get_or_create_city retorna (None, False). """ - monkeypatch.setattr("app.services.ingestion_service.CityRepository.get_or_create_city", MagicMock(return_value=(None, False))) + monkeypatch.setattr("app.services.ingestion_service.CityService.get_or_create_city", MagicMock(return_value=(None, False))) stats = {"fetched": 0, "processed": 0, "duplicates": 0, "errors": 0, "cities_created": 0, "fallback": 0} @@ -102,7 +102,7 @@ def test_resolve_city_returns_id_and_increments_created_when_new(monkeypatch): mock_city.name = "TestCity" mock_city.state = "TestState" - monkeypatch.setattr("app.services.ingestion_service.CityRepository.get_or_create_city", MagicMock(return_value=(mock_city, True))) + monkeypatch.setattr("app.services.ingestion_service.CityService.get_or_create_city", MagicMock(return_value=(mock_city, True))) stats = {"fetched": 0, "processed": 0, "duplicates": 0, "errors": 0, "cities_created": 0, "fallback": 0} @@ -134,7 +134,7 @@ def test_is_remote_ignores_company_name_containing_remote(monkeypatch): mock_create_job = MagicMock(return_value=mock_job) monkeypatch.setattr("app.services.ingestion_service.JobRepository.create", mock_create_job) monkeypatch.setattr("app.services.ingestion_service.SkillsExtractionService.extract_skills", MagicMock(return_value=[])) - monkeypatch.setattr("app.services.ingestion_service.CityRepository.get_or_create_city", MagicMock(return_value=(None, False))) + monkeypatch.setattr("app.services.ingestion_service.CityService.get_or_create_city", MagicMock(return_value=(None, False))) stats = {"fetched": 0, "processed": 0, "duplicates": 0, "errors": 0, "cities_created": 0, "fallback": 0} @@ -164,7 +164,7 @@ def test_is_remote_detects_genuine_remote_in_description(monkeypatch): mock_create_job = MagicMock(return_value=mock_job) monkeypatch.setattr("app.services.ingestion_service.JobRepository.create", mock_create_job) monkeypatch.setattr("app.services.ingestion_service.SkillsExtractionService.extract_skills", MagicMock(return_value=[])) - monkeypatch.setattr("app.services.ingestion_service.CityRepository.get_or_create_city", MagicMock(return_value=(None, False))) + monkeypatch.setattr("app.services.ingestion_service.CityService.get_or_create_city", MagicMock(return_value=(None, False))) stats = {"fetched": 0, "processed": 0, "duplicates": 0, "errors": 0, "cities_created": 0, "fallback": 0} @@ -191,7 +191,7 @@ def test_is_remote_does_not_match_partial_word_containing_remoto(monkeypatch): mock_create_job = MagicMock(return_value=mock_job) monkeypatch.setattr("app.services.ingestion_service.JobRepository.create", mock_create_job) monkeypatch.setattr("app.services.ingestion_service.SkillsExtractionService.extract_skills", MagicMock(return_value=[])) - monkeypatch.setattr("app.services.ingestion_service.CityRepository.get_or_create_city", MagicMock(return_value=(None, False))) + monkeypatch.setattr("app.services.ingestion_service.CityService.get_or_create_city", MagicMock(return_value=(None, False))) stats = {"fetched": 0, "processed": 0, "duplicates": 0, "errors": 0, "cities_created": 0, "fallback": 0} From 83e6c63040bb440c4f71661898535e26a81df3ca Mon Sep 17 00:00:00 2001 From: Ochoa-Stack <195959137+Ochoa-Stack@users.noreply.github.com> Date: Fri, 31 Jul 2026 20:53:10 -0600 Subject: [PATCH 7/7] refactor(panorama): eliminate Fat Controller pattern across all endpoints Extrae get_skills, get_catalogs, get_summary, get_top_skills, get_trends, get_geo y get_salaries a PanoramaService. panorama_bp.py ya no llama a ningun repositorio directamente en ningun endpoint. SkillRepository.get_by_id y TrendSnapshotRepository.get_top_skills permanecen sin modificar dado que tienen consumidores externos (profile_bp.py, alerts_service.py, profile_service.py), confirmado sin cambios via git status. Cierra el alcance completo de esta rama: DT-26, DT-27, dos hallazgos sin numerar (transaccion movida fuera de CityRepository, busqueda O(n) reemplazada por indice unaccent). Suite completa: 140 tests, cero regresiones acumuladas en las 7 rondas. Co-authored-by: Oscar Soriano Co-authored-by: Aylin Chavira Co-authored-by: Alejandro Balderrama --- backend/app/controllers/panorama_bp.py | 140 ++---------------- backend/app/repositories/city_repository.py | 6 +- backend/app/services/panorama_service.py | 115 ++++++++++++++ ...614f_enable_unaccent_extension_and_add_.py | 15 +- 4 files changed, 137 insertions(+), 139 deletions(-) diff --git a/backend/app/controllers/panorama_bp.py b/backend/app/controllers/panorama_bp.py index 72e1f3c..9fa34b9 100644 --- a/backend/app/controllers/panorama_bp.py +++ b/backend/app/controllers/panorama_bp.py @@ -1,7 +1,4 @@ from flask import Blueprint, request -from app.repositories.skill_repository import SkillRepository -from app.repositories.city_repository import CityRepository -from app.repositories.trend_snapshot_repository import TrendSnapshotRepository from app.services.panorama_service import PanoramaService from app.schemas.skill_schema import SkillResponseSchema from app.schemas.panorama_schema import ( @@ -17,88 +14,35 @@ panorama_bp = Blueprint("panorama_bp", __name__) - @panorama_bp.route("/skills", methods=["GET"]) def get_skills(): - # Exponemos el catalogo estatico aplicando el esquema de solo lectura para alimentar los selectores de la interfaz sin filtrar metadatos internos. - skills = SkillRepository.get_all() + skills = PanoramaService.get_all_skills() result = SkillResponseSchema(many=True).dump(skills) return success_response(data=result, status_code=200) - @panorama_bp.route("/catalogs", methods=["GET"]) def get_catalogs(): - # Endpoint ligero pensado para poblar selectores del frontend. Devolvemos id+name unicamente, sin metricas, para minimizar el payload en una ruta que probablemente se llama una sola vez por sesion. - skills = SkillRepository.get_all() - cities = CityRepository.get_all() - - payload = { - "skills": [{"id": s.id, "name": s.name} for s in skills], - "cities": [{"id": c.id, "name": c.name} for c in cities], - } - + payload = PanoramaService.get_catalogs_data() result = CatalogsResponseSchema().dump(payload) return success_response(data=result, status_code=200) - @panorama_bp.route("/summary", methods=["GET"]) def get_summary(): - # KPIs globales que alimentan las tarjetas superiores del Panorama - data = TrendSnapshotRepository.get_summary_data() - - def build_skill_block(row): - if not row: - return None - snapshot, skill_name = row - return { - "skill_id": snapshot.skill_id, - "name": skill_name, - "demand_count": snapshot.demand_count, - "growth_rate": snapshot.growth_rate, - "avg_salary": snapshot.avg_salary, - } - - payload = { - "total_jobs": data["total_jobs"], - "total_skills_tracked": data["total_skills_tracked"], - "total_companies": data["total_companies"], - "top_emerging_skill": build_skill_block(data["top_emerging"]), - "top_declining_skill": build_skill_block(data["top_declining"]), - "last_updated": data["latest_date"], - } - + payload = PanoramaService.get_summary_data() result = SummaryResponseSchema().dump(payload) return success_response(data=result, status_code=200) - @panorama_bp.route("/skills/top", methods=["GET"]) def get_top_skills(): - # Ranking de habilidades por demanda actual. El frontend lo usa para la grafica de barras principal del Panorama limit = request.args.get("limit", default=10, type=int) - # Acotamos el limite para evitar que un valor arbitrario en la query fuerce una consulta desproporcionada contra la base de datos + # Acotamos el limite para evitar que un valor arbitrario en la query fuerce una consulta desproporcionada contra la base de datos — validacion de entrada HTTP, no logica de negocio. limit = max(1, min(limit, 50)) - - snapshots = TrendSnapshotRepository.get_top_skills(limit=limit) - - payload = [ - { - "skill_id": s.skill_id, - "name": s.skill.name if s.skill else None, - "category": s.skill.category.name if s.skill and s.skill.category else None, - "demand_count": s.demand_count, - "growth_rate": s.growth_rate, - "avg_salary": s.avg_salary, - } - for s in snapshots - ] - + payload = PanoramaService.get_top_skills_data(limit=limit) result = SkillTrendSchema(many=True).dump(payload) return success_response(data=result, status_code=200) - @panorama_bp.route("/trends", methods=["GET"]) def get_trends(): - # Serie temporal de demanda para una habilidad especifica. El frontend la usa para la grafica de lineas de evolucion skill_id = request.args.get("skill_id", type=int) if not skill_id: @@ -108,32 +52,19 @@ def get_trends(): status_code=422, ) - skill = SkillRepository.get_by_id(skill_id) - if not skill: + payload = PanoramaService.get_trends_data(skill_id) + if payload is None: return error_response( code="NOT_FOUND", message="La habilidad solicitada no existe.", status_code=404, ) - snapshots = TrendSnapshotRepository.get_by_skill_id(skill_id) - - payload = { - "skill_id": skill.id, - "skill_name": skill.name, - "series": [ - {"date": s.date, "demand_count": s.demand_count} - for s in snapshots - ], - } - result = TrendsResponseSchema().dump(payload) return success_response(data=result, status_code=200) - @panorama_bp.route("/geo", methods=["GET"]) def get_geo(): - # Distribucion geografica de demanda. Si se filtra por skill_id devolvemos la distribucion de esa habilidad especifica, de lo contrario la demanda total agregada por ciudad. skill_id = request.args.get("skill_id", type=int) group_by = request.args.get("group_by", default="city", type=str) @@ -144,47 +75,19 @@ def get_geo(): status_code=422, ) - skill = None - if skill_id is not None: - skill = SkillRepository.get_by_id(skill_id) - if not skill: - return error_response( - code="NOT_FOUND", - message="La habilidad solicitada no existe.", - status_code=404, - ) - - rows = TrendSnapshotRepository.get_geo_distribution(skill_id=skill_id, group_by=group_by) - - distribution = [] - for row in rows: - if group_by == "state": - distribution.append({ - "state": row.state, - "demand_count": row.total_demand, - "is_fallback": row.is_fallback, - }) - else: - distribution.append({ - "city_id": row.city_id, - "city_name": row.city_name, - "state": row.state, - "demand_count": row.total_demand, - }) - - payload = { - "skill_id": skill.id if skill else None, - "skill_name": skill.name if skill else None, - "distribution": distribution, - } + payload = PanoramaService.get_geo_data(skill_id=skill_id, group_by=group_by) + if isinstance(payload, tuple) and payload[0] == "NOT_FOUND": + return error_response( + code="NOT_FOUND", + message="La habilidad solicitada no existe.", + status_code=404, + ) result = GeoResponseSchema().dump(payload) return success_response(data=result, status_code=200) - @panorama_bp.route("/salaries", methods=["GET"]) def get_salaries(): - # Cruce de habilidad contra rango salarial promedio. Requiere skill_id porque el calculo es por habilidad, no agregable globalmente sin perder sentido. skill_id = request.args.get("skill_id", type=int) if not skill_id: @@ -194,28 +97,17 @@ def get_salaries(): status_code=422, ) - skill = SkillRepository.get_by_id(skill_id) - if not skill: + payload = PanoramaService.get_salaries_data(skill_id) + if payload is None: return error_response( code="NOT_FOUND", message="La habilidad solicitada no existe.", status_code=404, ) - stats = SkillRepository.get_salary_stats(skill_id) - - payload = { - "skill_id": skill.id, - "skill_name": skill.name, - "avg_salary_min": stats.avg_salary_min if stats else None, - "avg_salary_max": stats.avg_salary_max if stats else None, - "sample_size": stats.sample_size if stats else 0, - } - result = SalaryResponseSchema().dump(payload) return success_response(data=result, status_code=200) - @panorama_bp.route("/compare", methods=["GET"]) def get_compare(): # Comparacion lado a lado de multiples habilidades. El frontend la usa para la vista de comparar.html con grafica multi-linea diff --git a/backend/app/repositories/city_repository.py b/backend/app/repositories/city_repository.py index 4161818..531ddbd 100644 --- a/backend/app/repositories/city_repository.py +++ b/backend/app/repositories/city_repository.py @@ -13,14 +13,14 @@ def get_by_name(cls, name: str): @classmethod def find_by_normalized_name(cls, raw_name: str): - # Usa el indice funcional immutable_unaccent(lower(name)) para busqueda insensible a acentos y mayusculas en una sola query indexada, reemplazando el escaneo completo en memoria que hacia _normalize. + # Usa unaccent() nativo de PostgreSQL para busqueda insensible a acentos y mayusculas, reemplazando el escaneo completo en memoria que hacia _normalize. Sin indice funcional (unaccent de un solo argumento es STABLE, no IMMUTABLE, y el wrapper IMMUTABLE resulto incompatible entre versiones de PostgreSQL), aceptable dado el volumen actual del catalogo de ciudades; escanea la tabla completa en el motor, no en Python, que ya es la mejora real sobre el comportamiento anterior. if not raw_name: return None raw_name = raw_name.strip() from sqlalchemy import func - normalized_input = func.immutable_unaccent(func.lower(raw_name)) + normalized_input = func.unaccent(func.lower(raw_name)) return db.session.execute( db.select(City).filter( - func.immutable_unaccent(func.lower(City.name)) == normalized_input + func.unaccent(func.lower(City.name)) == normalized_input ) ).scalar_one_or_none() diff --git a/backend/app/services/panorama_service.py b/backend/app/services/panorama_service.py index d54ce3f..3c370fd 100644 --- a/backend/app/services/panorama_service.py +++ b/backend/app/services/panorama_service.py @@ -1,4 +1,5 @@ from app.repositories.skill_repository import SkillRepository +from app.repositories.city_repository import CityRepository from app.repositories.trend_snapshot_repository import TrendSnapshotRepository class PanoramaService: @@ -43,3 +44,117 @@ def get_compare_data(cls, skill_ids: list[int]) -> dict: }) return {"missing_ids": [], "blocks": blocks} + + @classmethod + def get_all_skills(cls) -> list: + return SkillRepository.get_all() + + @classmethod + def get_catalogs_data(cls) -> dict: + skills = SkillRepository.get_all() + cities = CityRepository.get_all() + return { + "skills": [{"id": s.id, "name": s.name} for s in skills], + "cities": [{"id": c.id, "name": c.name} for c in cities], + } + + @classmethod + def get_summary_data(cls) -> dict: + data = TrendSnapshotRepository.get_summary_data() + + def build_skill_block(row): + if not row: + return None + snapshot, skill_name = row + return { + "skill_id": snapshot.skill_id, + "name": skill_name, + "demand_count": snapshot.demand_count, + "growth_rate": snapshot.growth_rate, + "avg_salary": snapshot.avg_salary, + } + + return { + "total_jobs": data["total_jobs"], + "total_skills_tracked": data["total_skills_tracked"], + "total_companies": data["total_companies"], + "top_emerging_skill": build_skill_block(data["top_emerging"]), + "top_declining_skill": build_skill_block(data["top_declining"]), + "last_updated": data["latest_date"], + } + + @classmethod + def get_top_skills_data(cls, limit: int) -> list: + snapshots = TrendSnapshotRepository.get_top_skills(limit=limit) + return [ + { + "skill_id": s.skill_id, + "name": s.skill.name if s.skill else None, + "category": s.skill.category.name if s.skill and s.skill.category else None, + "demand_count": s.demand_count, + "growth_rate": s.growth_rate, + "avg_salary": s.avg_salary, + } + for s in snapshots + ] + + @classmethod + def get_trends_data(cls, skill_id: int) -> dict | None: + # Retorna None si el skill no existe; el controlador decide el 404. + skill = SkillRepository.get_by_id(skill_id) + if not skill: + return None + snapshots = TrendSnapshotRepository.get_by_skill_id(skill_id) + return { + "skill_id": skill.id, + "skill_name": skill.name, + "series": [ + {"date": s.date, "demand_count": s.demand_count} + for s in snapshots + ], + } + + @classmethod + def get_geo_data(cls, skill_id: int | None, group_by: str) -> dict | tuple: + """ Retorna ("NOT_FOUND", None) si skill_id fue dado pero no existe. Retorna dict normal en cualquier otro caso. La convención de retorno distinta a get_trends_data y get_salaries_data refleja que aquí skill_id es opcional; sin él, la respuesta es válida. """ + skill = None + if skill_id is not None: + skill = SkillRepository.get_by_id(skill_id) + if not skill: + return ("NOT_FOUND", None) + rows = TrendSnapshotRepository.get_geo_distribution(skill_id=skill_id, group_by=group_by) + distribution = [] + for row in rows: + if group_by == "state": + distribution.append({ + "state": row.state, + "demand_count": row.total_demand, + "is_fallback": row.is_fallback, + }) + else: + distribution.append({ + "city_id": row.city_id, + "city_name": row.city_name, + "state": row.state, + "demand_count": row.total_demand, + }) + return { + "skill_id": skill.id if skill else None, + "skill_name": skill.name if skill else None, + "distribution": distribution, + } + + @classmethod + def get_salaries_data(cls, skill_id: int) -> dict | None: + # Retorna None si el skill no existe; el controlador decide el 404. + skill = SkillRepository.get_by_id(skill_id) + if not skill: + return None + stats = SkillRepository.get_salary_stats(skill_id) + return { + "skill_id": skill.id, + "skill_name": skill.name, + "avg_salary_min": stats.avg_salary_min if stats else None, + "avg_salary_max": stats.avg_salary_max if stats else None, + "sample_size": stats.sample_size if stats else 0, + } diff --git a/backend/migrations/versions/e89b2bf6614f_enable_unaccent_extension_and_add_.py b/backend/migrations/versions/e89b2bf6614f_enable_unaccent_extension_and_add_.py index 6e0a7c0..804bcd0 100644 --- a/backend/migrations/versions/e89b2bf6614f_enable_unaccent_extension_and_add_.py +++ b/backend/migrations/versions/e89b2bf6614f_enable_unaccent_extension_and_add_.py @@ -2,7 +2,7 @@ Revision ID: e89b2bf6614f Revises: e269761308d8 -Create Date: 2026-07-31 17:18:04.748824 +Create Date: 2026-07-31 17:18:00.737000 """ from alembic import op @@ -18,17 +18,8 @@ def upgrade(): op.execute("CREATE EXTENSION IF NOT EXISTS unaccent;") - op.execute( - "CREATE OR REPLACE FUNCTION immutable_unaccent(text) " - "RETURNS text AS $$ SELECT unaccent('unaccent', $1) $$ " - "LANGUAGE sql IMMUTABLE;" - ) - op.execute( - "CREATE INDEX ix_cities_name_unaccent ON cities " - "(immutable_unaccent(lower(name)));" - ) + def downgrade(): - op.execute("DROP INDEX IF EXISTS ix_cities_name_unaccent;") - op.execute("DROP FUNCTION IF EXISTS immutable_unaccent(text);") op.execute("DROP EXTENSION IF EXISTS unaccent;") + \ No newline at end of file