Skip to content

Fix unsafe row indexing and remove unnecessary Hash in stats endpoints#1218

Merged
adalpari merged 1 commit intotrunkfrom
adalpari/review-stats-endpoints
Mar 10, 2026
Merged

Fix unsafe row indexing and remove unnecessary Hash in stats endpoints#1218
adalpari merged 1 commit intotrunkfrom
adalpari/review-stats-endpoints

Conversation

@adalpari
Copy link
Copy Markdown
Contributor

@adalpari adalpari commented Mar 5, 2026

Description

Addresses two issues flagged in PR #1200 review that also exist in the current codebase. These improvements enhance safety and code quality across all stats endpoints.

Changes

  1. Fix unsafe row indexing in stats_visits.rs: The get_stats_data() function hard-coded row[0] to access the period field and used .unwrap() on position(). Now it safely looks up both the "period" and data field indices using position(), and uses .get() for bounds-safe access instead of direct indexing, which would panic on out-of-bounds access.

  2. Remove unnecessary Hash derives: Removed Hash from 12 stats Period/Unit enums and 8 DataPoint/Response types across all stats files. Analysis showed none of these types are used as HashMap/HashSet keys anywhere in the codebase—Hash was blindly copied from other types. The only type genuinely requiring Hash is AttachmentMetadataKey (used as a HashMap key in support_tickets.rs), which was correctly retained.

Test plan

  • All 1256 unit tests pass
  • cargo build succeeds with no errors
  • No new clippy warnings introduced (pre-existing warnings in other files remain)

…endpoints

Address two issues flagged in PR #1200 review:

1. Fix unsafe row indexing in stats_visits.rs: The get_stats_data() function
   hard-coded row[0] for period and used unwrap() on position(). Now it safely
   looks up both "period" and data field indices, using .get() for bounds-safe
   access instead of panicking on out-of-bounds indices.

2. Remove Hash derive from all stats types: Hash was blindly copied across
   12 stats Period/Unit enums and 8 DataPoint/Response types in stats_visits.rs,
   but none are used as HashMap/HashSet keys. Removed Hash from:
   - StatsVisitsUnit, StatsClicksPeriod, StatsReferrersPeriod, StatsSearchTermsPeriod,
     StatsTopAuthorsPeriod, StatsTopPostsPeriod, StatsCountryViewsPeriod,
     StatsRegionViewsPeriod, StatsCityViewsPeriod, StatsDevicesPeriod,
     StatsFileDownloadsPeriod, StatsVideoPlaysPeriod
   - StatsVisitsResponse, StatsVisitsDataPoint, StatsVisitorsDataPoint,
     StatsLikesDataPoint, StatsReblogsDataPoint, StatsCommentsDataPoint,
     StatsPostsDataPoint, StatsVisitsDataValue

All 1256 unit tests pass.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
@adalpari adalpari requested a review from jkmassel March 5, 2026 13:59
@wpmobilebot
Copy link
Copy Markdown
Collaborator

XCFramework Build

This PR's XCFramework is available for testing. Add to your Package.swift:

.package(url: "https://github.com/automattic/wordpress-rs", branch: "pr-build/1218")

Built from d19ed39

@adalpari adalpari merged commit c2105f7 into trunk Mar 10, 2026
33 checks passed
@adalpari adalpari deleted the adalpari/review-stats-endpoints branch March 10, 2026 08:12
adalpari added a commit that referenced this pull request Mar 12, 2026
…endpoints (#1218)

Address two issues flagged in PR #1200 review:

1. Fix unsafe row indexing in stats_visits.rs: The get_stats_data() function
   hard-coded row[0] for period and used unwrap() on position(). Now it safely
   looks up both "period" and data field indices, using .get() for bounds-safe
   access instead of panicking on out-of-bounds indices.

2. Remove Hash derive from all stats types: Hash was blindly copied across
   12 stats Period/Unit enums and 8 DataPoint/Response types in stats_visits.rs,
   but none are used as HashMap/HashSet keys. Removed Hash from:
   - StatsVisitsUnit, StatsClicksPeriod, StatsReferrersPeriod, StatsSearchTermsPeriod,
     StatsTopAuthorsPeriod, StatsTopPostsPeriod, StatsCountryViewsPeriod,
     StatsRegionViewsPeriod, StatsCityViewsPeriod, StatsDevicesPeriod,
     StatsFileDownloadsPeriod, StatsVideoPlaysPeriod
   - StatsVisitsResponse, StatsVisitsDataPoint, StatsVisitorsDataPoint,
     StatsLikesDataPoint, StatsReblogsDataPoint, StatsCommentsDataPoint,
     StatsPostsDataPoint, StatsVisitsDataValue

All 1256 unit tests pass.

Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>
adalpari added a commit that referenced this pull request Mar 12, 2026
…endpoints (#1218)

Address two issues flagged in PR #1200 review:

1. Fix unsafe row indexing in stats_visits.rs: The get_stats_data() function
   hard-coded row[0] for period and used unwrap() on position(). Now it safely
   looks up both "period" and data field indices, using .get() for bounds-safe
   access instead of panicking on out-of-bounds indices.

2. Remove Hash derive from all stats types: Hash was blindly copied across
   12 stats Period/Unit enums and 8 DataPoint/Response types in stats_visits.rs,
   but none are used as HashMap/HashSet keys. Removed Hash from:
   - StatsVisitsUnit, StatsClicksPeriod, StatsReferrersPeriod, StatsSearchTermsPeriod,
     StatsTopAuthorsPeriod, StatsTopPostsPeriod, StatsCountryViewsPeriod,
     StatsRegionViewsPeriod, StatsCityViewsPeriod, StatsDevicesPeriod,
     StatsFileDownloadsPeriod, StatsVideoPlaysPeriod
   - StatsVisitsResponse, StatsVisitsDataPoint, StatsVisitorsDataPoint,
     StatsLikesDataPoint, StatsReblogsDataPoint, StatsCommentsDataPoint,
     StatsPostsDataPoint, StatsVisitsDataValue

All 1256 unit tests pass.

Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>
adalpari added a commit that referenced this pull request Mar 18, 2026
* Add stats insights endpoint

Add support for the WP.com REST API v1.1 stats/insights endpoint,
which returns posting activity patterns including best hour/day to post,
hourly view breakdowns, and yearly posting summaries.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Handle empty array responses in stats insights deserialization

The WP.com API returns `[]` instead of `{}` for empty map fields
(days, hours, hourly_views). Use `deserialize_empty_array_or_hashmap`
to handle both cases gracefully.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Handle integer values in map deserialization for stats insights

The WP.com API returns `0` instead of `{}` for empty map fields
(days, hours, hourly_views) on some sites. Extend
`deserialize_empty_array_or_hashmap` to also accept integers,
treating them as empty HashMaps.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Make map deserializer handle all non-map placeholder values

The WP.com API can return various non-map values (0, null, false, [])
for map fields on sites with no data. Make deserialize_empty_array_or_hashmap
handle all of them by adding visit_bool, visit_none, visit_unit, and
visit_f64 handlers, returning empty HashMap for any non-map value.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Rewrite map deserializers to use serde_json::Value intermediate

Replace the visitor-based approach with a simpler Value-based strategy
that first deserializes any JSON value, then only converts JSON objects
into HashMaps. All non-object values (integers, null, false, arrays,
strings, etc.) are treated as empty map / None. This is more robust
against unexpected API response formats.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Add stats summary endpoint

Add GET /rest/v1.1/sites/<site_id>/stats endpoint that returns
aggregate site statistics and recent visit time-series data.
Reuses StatsVisitsResponse for the visits field. Supports
locale query parameter.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Add locale parameter to stats insights endpoint

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Bump fastlane-plugin-wpmreleasetoolkit from 14.1.0 to 14.2.0 (#1223)

Bumps [fastlane-plugin-wpmreleasetoolkit](https://github.com/wordpress-mobile/release-toolkit) from 14.1.0 to 14.2.0.
- [Release notes](https://github.com/wordpress-mobile/release-toolkit/releases)
- [Changelog](https://github.com/wordpress-mobile/release-toolkit/blob/trunk/CHANGELOG.md)
- [Commits](wordpress-mobile/release-toolkit@14.1.0...14.2.0)

---
updated-dependencies:
- dependency-name: fastlane-plugin-wpmreleasetoolkit
  dependency-version: 14.2.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump fluent-templates from 0.13.2 to 0.13.3 (#1222)

Bumps [fluent-templates](https://github.com/XAMPPRocky/fluent-templates) from 0.13.2 to 0.13.3.
- [Release notes](https://github.com/XAMPPRocky/fluent-templates/releases)
- [Changelog](https://github.com/XAMPPRocky/fluent-templates/blob/master/CHANGELOG.md)
- [Commits](XAMPPRocky/fluent-templates@fluent-templates-v0.13.2...fluent-templates-v0.13.3)

---
updated-dependencies:
- dependency-name: fluent-templates
  dependency-version: 0.13.3
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix unsafe row indexing and remove unnecessary Hash derives in stats endpoints (#1218)

Address two issues flagged in PR #1200 review:

1. Fix unsafe row indexing in stats_visits.rs: The get_stats_data() function
   hard-coded row[0] for period and used unwrap() on position(). Now it safely
   looks up both "period" and data field indices, using .get() for bounds-safe
   access instead of panicking on out-of-bounds indices.

2. Remove Hash derive from all stats types: Hash was blindly copied across
   12 stats Period/Unit enums and 8 DataPoint/Response types in stats_visits.rs,
   but none are used as HashMap/HashSet keys. Removed Hash from:
   - StatsVisitsUnit, StatsClicksPeriod, StatsReferrersPeriod, StatsSearchTermsPeriod,
     StatsTopAuthorsPeriod, StatsTopPostsPeriod, StatsCountryViewsPeriod,
     StatsRegionViewsPeriod, StatsCityViewsPeriod, StatsDevicesPeriod,
     StatsFileDownloadsPeriod, StatsVideoPlaysPeriod
   - StatsVisitsResponse, StatsVisitsDataPoint, StatsVisitorsDataPoint,
     StatsLikesDataPoint, StatsReblogsDataPoint, StatsCommentsDataPoint,
     StatsPostsDataPoint, StatsVisitsDataValue

All 1256 unit tests pass.

Co-authored-by: Claude Haiku 4.5 <noreply@anthropic.com>

* Bump chrono from 0.4.43 to 0.4.44 (#1221)

Bumps [chrono](https://github.com/chronotope/chrono) from 0.4.43 to 0.4.44.
- [Release notes](https://github.com/chronotope/chrono/releases)
- [Changelog](https://github.com/chronotope/chrono/blob/main/CHANGELOG.md)
- [Commits](chronotope/chrono@v0.4.43...v0.4.44)

---
updated-dependencies:
- dependency-name: chrono
  dependency-version: 0.4.44
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump proc-macro-crate from 3.4.0 to 3.5.0 (#1225)

Bumps [proc-macro-crate](https://github.com/bkchr/proc-macro-crate) from 3.4.0 to 3.5.0.
- [Release notes](https://github.com/bkchr/proc-macro-crate/releases)
- [Commits](bkchr/proc-macro-crate@v3.4.0...v3.5.0)

---
updated-dependencies:
- dependency-name: proc-macro-crate
  dependency-version: 3.5.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump quote from 1.0.44 to 1.0.45 (#1224)

Bumps [quote](https://github.com/dtolnay/quote) from 1.0.44 to 1.0.45.
- [Release notes](https://github.com/dtolnay/quote/releases)
- [Commits](dtolnay/quote@1.0.44...1.0.45)

---
updated-dependencies:
- dependency-name: quote
  dependency-version: 1.0.45
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Add a migration script to ensure custom terms are re-fetched (#1217)

* Fix loading trashed post by ids (#1226)

* Add integration tests for fetching trashed posts by id

* Fix loading trashed post by ids

* Add PostStatus::Any

* Support filtering by "any" status (#1210)

* Support filtering by "any" status

* Use PostStatus::Any

* Add stats tags endpoint

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* Revert changes to deserialize_empty_array_or_hashmap helpers

Restores the original Visitor-based implementation. These helpers
don't need to be modified for the stats tags endpoint.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Tony Li <tony.li@automattic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants