Skip to content

Conversation

mjankowski
Copy link
Contributor

Same background as #32915 and same guidance (ie, maybe some framework-generation style diffs, but should be same logic). Nudging things towards greater consistency/composability across the app.

@mjankowski mjankowski added performance Runtime performance refactoring Improving code quality labels Nov 16, 2024
@ClearlyClaire
Copy link
Contributor

Is there a reason to be using arrays for [:id] instead of just :id?

@mjankowski
Copy link
Contributor Author

Is there a reason to be using arrays for [:id] instead of just :id?

Ah, I meant to include this in PR desc, but yes there is. The narrow answer is "it works that way but not the flat hash way". As to WHY - I don't know yet. Example from the AccountFilter changes...

With the flat hash style, an error like...

An error occurred when inspecting the object: #<ActiveRecord::StatementInvalid:"PG::SyntaxError: ERROR:  syntax error at or near \"AS\"\nLINE 1: ... $2) GROUP BY \"accounts\".\"id\", \"accounts\".\"users\" AS id, \"ac...\n                                                             ^\n">

...shows up (note the ...AS... portion in there ... I suspect the symbol is getting handled by different code path which pulls that out). But with the nested array style it works correctly (ie, that relevant section comes through as GROUP BY "users"."id", "accounts"."id" for example...)

I'm going to do a follow-up here and see if there's upstream reason and/or maybe this one is available in Rails 8 but not 7.2, maybe it isn't fully converted yet, etc...

I recall thinking there might be some subtlety around being called from the base class or an association vs being called from a more built-up query with joins from elsewhere, but didn't go deep enough to confirm.

So I guess - feel free to merge as-is with that explanation and I'll do follow-up PR if I sort it out, or leave open for now and I'll report back here if I sort it out.

Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

ClearlyClaire
ClearlyClaire previously approved these changes Nov 19, 2024
Copy link
Contributor

This pull request has resolved merge conflicts and is ready for review.

@ClearlyClaire ClearlyClaire added this pull request to the merge queue Nov 19, 2024
Merged via the queue into mastodon:main with commit 37f00fb Nov 19, 2024
27 checks passed
@mjankowski mjankowski deleted the rails-7-2-hash-group branch November 19, 2024 10:51
rezhajulio added a commit to PegelinuxTop/mastodon that referenced this pull request Nov 26, 2024
* Remove remnants of embed views (mastodon#32419)

* Use `with_options` blocks in `Associations` concern (mastodon#32840)

* Add OpenTelemetry VCS attributes (mastodon#32904)

* Add missing autofocus on boost modal (mastodon#32953)

* Update libretranslate/libretranslate Docker tag to v1.6.2 (mastodon#32951)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Update dependency @formatjs/cli to v6.3.11 (mastodon#32950)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* New Crowdin Translations (automated) (mastodon#32927)

Co-authored-by: GitHub Actions <noreply@github.com>

* `Form::Import` spec reduce factories (mastodon#32924)

* Reduce long lines in `relationships/show` view (mastodon#32922)

* Extract constants for controller `LIMIT` queries (mastodon#32921)

* Update dependency aws-sdk-s3 to v1.171.0 (mastodon#32901)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Use `nil` instead of `false` for datetime value in spec (mastodon#32926)

* Streaming: add development logging of database queries (mastodon#32945)

* Fix `min_id` and `max_id` causing error in search API (mastodon#32857)

* Move self destruct check to `config_for` and add constant for verifier string (mastodon#32943)

* Change dependency from annotate to annotaterb (mastodon#30232)

* Use hash arguments to `select` when possible (Rails 7.2) (mastodon#32917)

* Use hash arguments to `order` when possible (Rails 7.2) (mastodon#32915)

* Add coverage for fixing the thing with the dates in the search area (mastodon#32958)

* Prep work for coverage addition to `ActivityPub::DeliveryWorker` spec (mastodon#32944)

* Use group/count approach in annual report classes (mastodon#32914)

* DB Replica database_tasks as ENV (mastodon#32908)

* Fix notification polling showing a loading bar in web UI (mastodon#32960)

* Update dependency aws-sdk-s3 to v1.172.0 (mastodon#32964)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* New Crowdin Translations (automated) (mastodon#32969)

Co-authored-by: GitHub Actions <noreply@github.com>

* Add `LoginActivity` model spec and `BrowserDetection` concern shared example (mastodon#32959)

* Extract `Notification::Groups` concern (mastodon#32968)

* Change lists to reflect added and removed users retroactively (mastodon#32930)

* Use hash arguments to `group` when possible (mastodon#32916)

* Fix error when viewing statuses to deleted replies in moderation view (mastodon#32986)

* Update dependency opentelemetry-instrumentation-rails to '~> 0.33.0' (mastodon#32987)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Add `AccountWarning` case to `Report#history` spec (mastodon#32984)

* New Crowdin Translations (automated) (mastodon#32990)

Co-authored-by: GitHub Actions <noreply@github.com>

* Update dependency json-schema to v5.1.0 (mastodon#32992)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Fix posts made in the future being allowed to trend (mastodon#32996)

* Add ability to middle-mouse click posts in web UI (mastodon#32988)

* block scheduled status from frozen users (mastodon#30729)

* Fix wrong video dimensions for some rotated videos (mastodon#33008)

* New Crowdin Translations (automated) (mastodon#33010)

Co-authored-by: GitHub Actions <noreply@github.com>

* Update dependency doorkeeper to v5.8.0 (mastodon#33000)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Add stop-gap antispam code (mastodon#32981)

* `rel="me"` check should be case-insenstive (mastodon#32238)

* Change design of lists in web UI (mastodon#32881)

* Fix duplicate notifications in notification groups when using slow mode (mastodon#33014)

* improve nginx.conf by setting gzip_static to on (mastodon#30698)

* LinkDetailsExtractor adjustments (mastodon#31357)

* Fix pushing hashtag-followed posts to feeds of inactive users (mastodon#33018)

* Rely on Rails to enable YJIT when available (mastodon#33017)

* Update Yarn to v4.5.2 (mastodon#33021)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* New Crowdin Translations (automated) (mastodon#33020)

Co-authored-by: GitHub Actions <noreply@github.com>

* Update dependency aws-sdk-s3 to v1.173.0 (mastodon#33019)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* [Glitch] Add missing autofocus on boost modal

Port 60801ab to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Fix notification polling showing a loading bar in web UI

Port e856838 to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* Set autofocus on favourite modal (glitch-soc#2900)

* New Crowdin Translations (automated) (glitch-soc#2896)

* New Crowdin translations

* Fix bogus no.yml

* Fix bogus simple_form.no.yml

---------

Co-authored-by: GitHub Actions <noreply@github.com>
Co-authored-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Fix duplicate notifications in notification groups when using slow mode

Port 4bfb888 to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* Remove pinned accounts editor

It was utterly broken and the underlying modals are getting redesigned

* [Glitch] Change design of lists in web UI

Port 6260350 to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* Fix issue templates (glitch-soc#2904)

---------

Signed-off-by: Claire <claire.github-309c@sitedethib.com>
Co-authored-by: Matt Jankowski <matt@jankowski.online>
Co-authored-by: Renaud Chaput <renchap@gmail.com>
Co-authored-by: Jeong Arm <kjwonmail@gmail.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: GitHub Actions <noreply@github.com>
Co-authored-by: Emelia Smith <ThisIsMissEm@users.noreply.github.com>
Co-authored-by: Eugen Rochko <eugen@zeonfederated.com>
Co-authored-by: Andrew W. Lee <wjldrew@gmail.com>
Co-authored-by: Shlee <github@shl.ee>
Co-authored-by: Claire <claire.github-309c@sitedethib.com>
Co-authored-by: Paulo Vilarinho <33267902+PauloVilarinho@users.noreply.github.com>
Co-authored-by: Christian Schmidt <github@chsc.dk>
Co-authored-by: Joshix-1 <57299889+Joshix-1@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Runtime performance refactoring Improving code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants