diff --git a/CHANGELOG.md b/CHANGELOG.md index 16d229d..066955c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,44 @@ All notable changes to the USDA FDC Python Client will be documented in this file. +## [Unreleased] + +### Security +- The API key is now sent as an `X-Api-Key` header instead of an `api_key` query + parameter, and is redacted from error messages. `requests` embeds the full URL + in its exception text, so a key in the query string was written into the + caller's tracebacks, log files and error tracker the first time the network + hiccuped: + + ``` + FdcApiError: Request failed: ... Max retries exceeded with url: + /fdc/v1/food/1750340?format=full&api_key=YOUR_REAL_KEY + ``` + + No API change: `FdcClient(api_key=...)` and `FDC_API_KEY` work as before. + +### Fixed +- **A food's kilojoules could be reported as its calories.** FDC lists energy + several times per food under the same name — `1008` (kcal), `1062` (the same + energy in kJ), and the Atwater variants `2047`/`2048` (kcal). All of them were + matched on the name alone and keyed as `calories`, so whichever came last in + the list won. Clarified butter (`fdc_id` 171314) lists 900 kcal and then + 3766 kJ, and `analyze_food` reported **3766 calories** — which the CLI and the + HTML report then printed with a `kcal` suffix. + + Energy is now matched on its nutrient id/number and unit: only kcal rows can + become `calories`, the plain `Energy` row is preferred over the Atwater + variants so the result never depends on list order, and the kJ figure is kept + under `energy_kj` rather than discarded. + +- **`format="abridged"` silently returned a food with no nutrients.** An + abridged response inlines each nutrient's fields on the row and carries no + nutrient id, while `format="full"` nests them under a `nutrient` key. Only the + nested shape was parsed, and the rest were dropped without a word: ghee came + back with 0 nutrients instead of 18. Both shapes are now understood, so + anyone reaching for abridged to save bandwidth no longer computes nutrition + from an empty list. + ## [0.1.10] - 2026-07-13 ### Added diff --git a/tests/unit/test_analysis_energy.py b/tests/unit/test_analysis_energy.py new file mode 100644 index 0000000..4429cd7 --- /dev/null +++ b/tests/unit/test_analysis_energy.py @@ -0,0 +1,101 @@ +""" +Tests for how a food's energy is turned into "calories". + +FDC reports energy several times per food, under the same name, in different +units and by different derivations: + + 1008 / 208 Energy kcal + 1062 / 268 Energy kJ (the same energy!) + 2047 / 957 Energy (Atwater General Factors) kcal + 2048 / 958 Energy (Atwater Specific Factors) kcal + +Matching those on the name alone made every one of them "calories", so whichever +came last in the list won — and the CLI and HTML report printed the winner with a +kcal suffix regardless of what unit it was actually in. +""" + +import pytest + +from usda_fdc.models import Food, Nutrient +from usda_fdc.analysis import analyze_food + + +def _food(*nutrients: Nutrient) -> Food: + return Food( + fdc_id=171314, + description="Butter, Clarified butter (ghee)", + data_type="SR Legacy", + nutrients=list(nutrients), + ) + + +ENERGY_KCAL = Nutrient(id=1008, name="Energy", amount=900.0, unit_name="kcal", nutrient_nbr="208") +ENERGY_KJ = Nutrient(id=1062, name="Energy", amount=3766.0, unit_name="kJ", nutrient_nbr="268") +ATWATER_GENERAL = Nutrient(id=2047, name="Energy (Atwater General Factors)", amount=64.66, unit_name="kcal", nutrient_nbr="957") +ATWATER_SPECIFIC = Nutrient(id=2048, name="Energy (Atwater Specific Factors)", amount=58.20, unit_name="kcal", nutrient_nbr="958") + + +def test_kilojoules_are_not_reported_as_calories(): + """The regression: real fdc_id 171314 lists 900 kcal and then 3766 kJ, and + was analyzed as 3766 "calories" — a 4.2x overstatement of a food's energy.""" + analysis = analyze_food(_food(ENERGY_KCAL, ENERGY_KJ)) + + assert analysis.calories_per_serving == 900.0 + + +def test_kilojoule_row_does_not_win_by_coming_last(): + """Order is the whole bug: the kJ row overwrote the kcal row simply by being + later in the list.""" + analysis = analyze_food(_food(ENERGY_KJ, ENERGY_KCAL)) + assert analysis.calories_per_serving == 900.0 + + analysis = analyze_food(_food(ENERGY_KCAL, ENERGY_KJ)) + assert analysis.calories_per_serving == 900.0 + + +def test_kilojoules_are_still_available_under_their_own_id(): + """Keyed separately rather than dropped, so no data is lost.""" + analysis = analyze_food(_food(ENERGY_KCAL, ENERGY_KJ)) + + kj = analysis.get_nutrient("energy_kj") + assert kj is not None + assert kj.amount == 3766.0 + assert kj.unit == "kJ" + + +def test_calories_is_the_kcal_row_even_when_only_kilojoules_and_atwater_exist(): + analysis = analyze_food(_food(ENERGY_KJ, ATWATER_SPECIFIC)) + + assert analysis.calories_per_serving == 58.20 + + +def test_competing_kcal_rows_resolve_the_same_way_regardless_of_order(): + """Real foods (fdc_id 1750340) carry both Atwater rows with different values. + Whichever we prefer, it must not depend on the order FDC happened to send.""" + forwards = analyze_food(_food(ATWATER_GENERAL, ATWATER_SPECIFIC)) + backwards = analyze_food(_food(ATWATER_SPECIFIC, ATWATER_GENERAL)) + + assert forwards.calories_per_serving == backwards.calories_per_serving + + +def test_plain_energy_row_is_preferred_over_atwater_variants(): + analysis = analyze_food(_food(ATWATER_GENERAL, ATWATER_SPECIFIC, ENERGY_KCAL)) + + assert analysis.calories_per_serving == 900.0 + + +def test_energy_is_matched_on_an_abridged_food_which_has_no_nutrient_id(): + """An abridged response carries only the nutrient number, and spells the unit + "KCAL" rather than "kcal".""" + abridged_kcal = Nutrient(id=None, name="Energy", amount=900.0, unit_name="KCAL", nutrient_nbr="208") + abridged_kj = Nutrient(id=None, name="Energy", amount=3770.0, unit_name="kJ", nutrient_nbr="268") + + analysis = analyze_food(_food(abridged_kj, abridged_kcal)) + + assert analysis.calories_per_serving == 900.0 + + +def test_calories_scale_with_serving_size(): + analysis = analyze_food(_food(ENERGY_KCAL, ENERGY_KJ), serving_size=50.0) + + assert analysis.calories_per_serving == 450.0 diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 142dce2..996d00e 100644 --- a/tests/unit/test_client.py +++ b/tests/unit/test_client.py @@ -181,3 +181,58 @@ def test_timeout_is_distinguishable_from_other_failures(): client._make_request("foods/search") assert not isinstance(exc.value, FdcTimeoutError) + + +# ── API key handling ────────────────────────────────────────────────── +# The key used to travel as a query parameter. requests puts the full URL into +# its exception messages, so the first connection blip wrote the caller's key +# into their tracebacks, log files and error tracker. + +SECRET = "SUPER_SECRET_KEY_12345" + + +def test_api_key_is_sent_as_a_header(): + client = FdcClient(api_key=SECRET) + assert client.session.headers["X-Api-Key"] == SECRET + + +def test_api_key_is_not_put_in_the_query_string(): + """The source of the leak: a key in the URL is a key in every log that URL + reaches.""" + client = FdcClient(api_key=SECRET) + + with patch.object(client.session, "request") as mock_request: + mock_request.return_value = MagicMock( + status_code=200, + json=MagicMock(return_value={}), + raise_for_status=MagicMock(), + ) + client._make_request("foods/search") + + assert "api_key" not in mock_request.call_args.kwargs["params"] + + +def test_api_key_does_not_leak_into_a_network_error(): + client = FdcClient(api_key=SECRET) + + leaky = requests.exceptions.ConnectionError( + f"Max retries exceeded with url: /fdc/v1/food/1?api_key={SECRET}" + ) + with patch.object(client.session, "request", side_effect=leaky): + with pytest.raises(FdcApiError) as exc: + client._make_request("food/1") + + assert SECRET not in str(exc.value) + + +def test_api_key_does_not_leak_into_an_http_error_body(): + client = FdcClient(api_key=SECRET) + + response = MagicMock(status_code=500, text=f"upstream rejected api_key={SECRET}") + response.raise_for_status.side_effect = requests.exceptions.HTTPError("500") + + with patch.object(client.session, "request", return_value=response): + with pytest.raises(FdcApiError) as exc: + client._make_request("food/1") + + assert SECRET not in str(exc.value) diff --git a/tests/unit/test_models.py b/tests/unit/test_models.py index fa91e63..df07864 100644 --- a/tests/unit/test_models.py +++ b/tests/unit/test_models.py @@ -139,3 +139,70 @@ def test_search_result_food_without_gtin_upc_is_none(): }) assert result.gtin_upc is None + + +# ── Abridged responses ──────────────────────────────────────────────── +# FDC describes a nutrient two ways. format=full nests it under a "nutrient" +# key; format=abridged inlines the fields on the row and carries no nutrient id, +# only its number. Parsing only the nested shape meant every nutrient of an +# abridged food was silently dropped: a food with 35 nutrients came back with 0, +# and no error to say so. + +ABRIDGED_NUTRIENT = { + "number": "208", + "name": "Energy", + "amount": 900.0, + "unitName": "KCAL", + "derivationCode": "LC", +} + +FULL_NUTRIENT = { + "nutrient": {"id": 1008, "name": "Energy", "unitName": "kcal", "number": "208", "rank": 300}, + "amount": 900.0, +} + + +def test_nutrient_parses_the_abridged_inline_shape(): + nutrient = Nutrient.from_api_data(ABRIDGED_NUTRIENT) + + assert nutrient.name == "Energy" + assert nutrient.amount == 900.0 + assert nutrient.unit_name == "KCAL" + assert nutrient.nutrient_nbr == "208" + + +def test_nutrient_still_parses_the_full_nested_shape(): + nutrient = Nutrient.from_api_data(FULL_NUTRIENT) + + assert nutrient.id == 1008 + assert nutrient.name == "Energy" + assert nutrient.amount == 900.0 + assert nutrient.unit_name == "kcal" + + +def test_abridged_food_keeps_its_nutrients(): + """The regression: a real abridged food (fdc_id 171314) has 18 nutrients and + was parsed into a Food with none, silently. Anyone computing nutrition from + it got zeros.""" + food = Food.from_api_data({ + "fdcId": 171314, + "description": "Butter, Clarified butter (ghee)", + "dataType": "SR Legacy", + "foodNutrients": [ABRIDGED_NUTRIENT], + }) + + assert len(food.nutrients) == 1 + assert food.nutrients[0].amount == 900.0 + assert food.nutrients[0].unit_name == "KCAL" + + +def test_full_food_still_keeps_its_nutrients(): + food = Food.from_api_data({ + "fdcId": 171314, + "description": "Butter, Clarified butter (ghee)", + "dataType": "SR Legacy", + "foodNutrients": [FULL_NUTRIENT], + }) + + assert len(food.nutrients) == 1 + assert food.nutrients[0].id == 1008 diff --git a/usda_fdc/analysis/analysis.py b/usda_fdc/analysis/analysis.py index eeed568..b0b1f8d 100644 --- a/usda_fdc/analysis/analysis.py +++ b/usda_fdc/analysis/analysis.py @@ -46,16 +46,69 @@ def get_nutrient(self, nutrient_id: str) -> Optional[NutrientValue]: """ return self.nutrients.get(nutrient_id.lower()) +# FDC reports a food's energy several times over, under the same name: +# +# 1008 / 208 Energy kcal +# 1062 / 268 Energy kJ (the same energy!) +# 2047 / 957 Energy (Atwater General Factors) kcal +# 2048 / 958 Energy (Atwater Specific Factors) kcal +# +# Matching those on the name alone made every one of them "calories", so +# whichever came last in the list won. Clarified butter (fdc_id 171314) lists +# 900 kcal and then 3766 kJ, and was reported as 3766 "calories" — a figure the +# CLI and the HTML report then printed with a kcal suffix. +# +# Only the kcal rows are calories, and where a food carries more than one of +# them we prefer the plain Energy row so the answer cannot depend on list order. +_ENERGY_KCAL_PRECEDENCE = ( + (1008, "208"), # Energy + (2048, "958"), # Energy (Atwater Specific Factors) + (2047, "957"), # Energy (Atwater General Factors) +) + +# Abridged responses spell the unit "KCAL"; full ones spell it "kcal". +_KCAL_UNITS = {"kcal", "kilocalorie", "kilocalories"} + + +def _is_energy(nutrient: Nutrient) -> bool: + """Whether a nutrient row describes a food's energy, in any unit.""" + return "energy" in (nutrient.name or "").lower() + + +def _is_kcal(nutrient: Nutrient) -> bool: + """Whether a nutrient row is measured in kilocalories rather than kilojoules.""" + return (nutrient.unit_name or "").strip().lower() in _KCAL_UNITS + + +def _energy_precedence(nutrient: Nutrient) -> int: + """Rank an energy row; lower wins. + + An abridged food carries no nutrient id, only its number, so match on + either. + """ + for rank, (nutrient_id, nutrient_nbr) in enumerate(_ENERGY_KCAL_PRECEDENCE): + if nutrient.id == nutrient_id or str(nutrient.nutrient_nbr or "") == nutrient_nbr: + return rank + return len(_ENERGY_KCAL_PRECEDENCE) + + def _get_nutrient_id(nutrient: Nutrient) -> str: """ Get a standardized nutrient ID from a nutrient. - + Args: nutrient: The nutrient object. - + Returns: A standardized nutrient ID. """ + if _is_energy(nutrient): + # A kJ row is not calories. Give it an id of its own so it cannot + # overwrite the kcal row it sits beside. + if _is_kcal(nutrient): + return "calories" + return f"energy_{(nutrient.unit_name or 'unknown').strip().lower()}" + # Map common nutrient names to standardized IDs name_map = { "protein": "protein", @@ -70,8 +123,7 @@ def _get_nutrient_id(nutrient: Nutrient) -> str: "vitamin c, total ascorbic acid": "vitamin_c", "vitamin a, iu": "vitamin_a", "cholesterol": "cholesterol", - "potassium, k": "potassium", - "energy": "calories" + "potassium, k": "potassium" } # Try to match by name @@ -109,22 +161,26 @@ def analyze_food( serving_size=serving_size ) + # Rank of the energy row currently holding "calories", so a lower-ranked + # one cannot displace it. + calories_rank: Optional[int] = None + # Process nutrients for nutrient in food.nutrients: # Get standardized nutrient ID nutrient_id = _get_nutrient_id(nutrient) - + # Calculate amount for the serving size amount = nutrient.amount * (serving_size / 100.0) - + # Get DRI value if available dri_value = get_dri(nutrient_id, dri_type, gender, age) - + # Calculate DRI percentage if DRI is available dri_percent = None if dri_value is not None and dri_value > 0: dri_percent = (amount / dri_value) * 100.0 - + # Create nutrient value nutrient_value = NutrientValue( nutrient=nutrient, @@ -134,10 +190,17 @@ def analyze_food( dri_percent=dri_percent, dri_type=dri_type ) - + + if nutrient_id == "calories": + rank = _energy_precedence(nutrient) + if calories_rank is not None and rank >= calories_rank: + # Already holding a better energy row; keep it. + continue + calories_rank = rank + # Add to nutrients dictionary analysis.nutrients[nutrient_id] = nutrient_value - + # Track macronutrients if nutrient_id == "protein": analysis.protein_per_serving = amount diff --git a/usda_fdc/client.py b/usda_fdc/client.py index 3d100f7..4212576 100644 --- a/usda_fdc/client.py +++ b/usda_fdc/client.py @@ -67,7 +67,19 @@ def __init__( self.base_url = base_url self.timeout = timeout self.session = requests.Session() - + + # Send the key as a header, not a query parameter. requests puts the + # full URL in its exception messages, so a key in the query string ends + # up in tracebacks, log files and error trackers the first time the + # network hiccups. api.data.gov, which fronts FDC, accepts either. + self.session.headers.update({"X-Api-Key": self.api_key}) + + def _redact(self, text: str) -> str: + """Remove the API key from text on its way into an exception or a log.""" + if self.api_key and self.api_key in text: + return text.replace(self.api_key, "***REDACTED***") + return text + def _make_request( self, endpoint: str, @@ -93,11 +105,9 @@ def _make_request( FdcApiError: For other API errors. """ url = urljoin(self.base_url, endpoint) - - # Always include the API key in the parameters + params = params or {} - params["api_key"] = self.api_key - + try: response = self.session.request( method=method, @@ -116,15 +126,17 @@ def _make_request( elif response.status_code == 429: raise FdcRateLimitError("Rate limit exceeded.") from e else: - raise FdcApiError(f"API error: {response.status_code} - {response.text}") from e + raise FdcApiError( + f"API error: {response.status_code} - {self._redact(response.text)}" + ) from e except requests.exceptions.Timeout as e: # Must precede RequestException: Timeout is a subclass of it, and a # timeout is worth retrying where a 400 is not. raise FdcTimeoutError( - f"Request to {url} timed out after {self.timeout}s" + f"Request to {self._redact(url)} timed out after {self.timeout}s" ) from e except requests.exceptions.RequestException as e: - raise FdcApiError(f"Request failed: {str(e)}") from e + raise FdcApiError(f"Request failed: {self._redact(str(e))}") from e def search( self, diff --git a/usda_fdc/models.py b/usda_fdc/models.py index 6fc6d7a..bac02b2 100644 --- a/usda_fdc/models.py +++ b/usda_fdc/models.py @@ -27,21 +27,43 @@ def __str__(self) -> str: def from_api_data(cls, data: Dict[str, Any]) -> 'Nutrient': """ Create a Nutrient instance from API data. - + + FDC describes a nutrient two different ways. ``format=full`` nests it + under a ``nutrient`` key, while ``format=abridged`` and the search + endpoint inline the fields on the row itself. Both are handled here: + reading only the nested shape silently dropped every nutrient of an + abridged food. + Args: data: The API response data for a nutrient. - + Returns: A Nutrient instance. """ - nutrient_data = data.get('nutrient', {}) + nutrient_data = data.get('nutrient') + if nutrient_data: + return cls( + id=nutrient_data.get('id'), + name=nutrient_data.get('name'), + amount=data.get('amount', 0), + unit_name=nutrient_data.get('unitName'), + nutrient_nbr=nutrient_data.get('number'), + rank=nutrient_data.get('rank') + ) + + # Inline shape. Abridged foods carry no nutrient id, only its number, + # and the search endpoint names the amount "value". + amount = data.get('amount') + if amount is None: + amount = data.get('value', 0) + return cls( - id=nutrient_data.get('id'), - name=nutrient_data.get('name'), - amount=data.get('amount', 0), - unit_name=nutrient_data.get('unitName'), - nutrient_nbr=nutrient_data.get('number'), - rank=nutrient_data.get('rank') + id=data.get('nutrientId'), + name=data.get('nutrientName') or data.get('name'), + amount=amount, + unit_name=data.get('unitName'), + nutrient_nbr=data.get('nutrientNumber') or data.get('number'), + rank=data.get('rank') ) @@ -119,8 +141,10 @@ def from_api_data(cls, data: Dict[str, Any], abridged: bool = False) -> 'Food': Args: data: The API response data for a food. - abridged: Whether the data is in abridged format. - + abridged: Whether the data is in abridged format. Nutrients are + parsed either way; this only skips food portions, which an + abridged response does not carry. + Returns: A Food instance. """ @@ -141,14 +165,15 @@ def from_api_data(cls, data: Dict[str, Any], abridged: bool = False) -> 'Food': household_serving_fulltext=data.get('householdServingFullText') ) - # Parse nutrients if present - if 'foodNutrients' in data and not abridged: + # Parse nutrients if present. Nutrient.from_api_data understands both + # the nested (full) and inline (abridged) shapes, so an abridged food + # keeps its nutrients instead of coming back silently empty. + if 'foodNutrients' in data: food.nutrients = [ - Nutrient.from_api_data(nutrient) - for nutrient in data['foodNutrients'] - if 'nutrient' in nutrient + Nutrient.from_api_data(nutrient) + for nutrient in data['foodNutrients'] ] - + # Parse food portions if present if 'foodPortions' in data and not abridged: food.food_portions = [