Add moderation zones where anonymous notes are not allowed

[pablobm]

Fixes #6570 (Enable PostGIS).

This PR introduces "moderation zones" where anonymous notes are not allowed. It also enables PostGIS in the process.

This addresses openstreetmap/operations#1362 partially:

• Within the moderation zones, only unauthenticated users are blocked, not "recent" ones. We can measure the success of this one before moving on to recent accounts which are a bit more complex.
• No UI for moderators to define the zones. This will be implemented in a future PR.

Interacting with PostGIS

In PostGIS terms, these zones are defined as geometries as opposed to geographies. This is consistent with the rest of the API.

Two gem options are available:

• activerecord-postgis-adapter:
  • "Classic" gem, 15 years old.
  • Requires setting adapter: postgis which can be a headache when moving between branches.
  • Doesn't work well when using geography-enabled functions such as ST_Covers. Probably can be added by implementing the operations with Arel.
  • Does work well with ST_Contains.
  • Works with Ruby 3.2, probably lower but I haven't checked.
• activerecord-postgis (current choice):
  • Very recent gem, 1 year old, I only discovered it by accident.
  • Doesn't require changing config/database.yml.
  • Works with geography-enabled functions such as ST_Covers by interpolating params into ?, but not with Arel (can be added though).
  • Requires Ruby 3.3 minimum (although as mentioned Ruby 3.2 is about to enter EOL).
  • Claims not to be based on monkeypatching in comparison to the competition (I haven't looked into this).

In common to both:

• They are drop-in replacements for each other, at least as long as you stick to geometry functions.
• Both support Arel queries for geometry, which is the way I have implemented the query.
• They use RGeo behind the scenes to encapsulate GIS objects. This requires creating them with one of several available factories. I have gone for RGeo::Cartesian.simple_factory as it appears to just work. If we changed to one of the others, we'd need the gem ffi-geos. This is not made clear in the documentation.

I haven't found a big argument for either, but I have gone for the newer one if only because of the simplicity of not having to update config/database.yml.

Future work

• If an unauthenticated user writes a long, thoughtful note, submits it, but then it fails because it was a moderation zone... they have wasted their time, if not lost their work. Therefore we may want a way for clients to tell ahead of time if a specific location has a block. Probably an API such as GET /api/0.6/notes/allowed?lat=X&lon=Y.
• There's no UI to define the zones. We'll need a CRUD for moderators to use.

[mmd-osm]

Geoblocking usually refers to a different concept, i.e. a block based on the geographical location of a user, rather than an area they aren't supposed to edit in. I believe it would avoid confusion if this feature were given a different name.

[pablobm]

it would avoid confusion if this feature were given a different name.

Good call. Happy to hear suggestions 🙂

[gravitystorm]

Initial thoughts inline.

On a general overview, I'm happy with the scope of this initial PR, and then following up with a separate PR to sort out the "detection api" topic, then actually creating the zones via a UI.

[gravitystorm]

You asked about which gem to use, but I don't have a strong opinion on this, other people might have more experience with the alternative gem. It would be good to check if they are easily interchangeable - i.e. is the reason for their difference more to do with monkeypatching, or do they also have e.g. differences in how to write the queries?

[pablobm]

This is what I have so far:

1. activerecord-postgis-adapter: this seems to be the popular choice, but I'm having issues with it and barely can make it work. Also I find it annoying that I need to change the DB config to adapter: postgis, as then I have to change it back when I move branches to look at something else.
2. activerecord-postgis: it works and doesn't have the adapter: annoyance. However it requires Ruby 3.3 minimum.

For now I have gone with option 2. I need to research what's wrong with option 1 and whether I'm doing something wrong.

The differences appear to be with the implementation, but I haven't looked much into it yet. They seem to be interchangeable, with queries written pretty much the seem way and both relying on rgeo-activerecord.

[tomhughes]

Ruby 3.2 is EOL at the end of March in any case so we probably need to be thinking about moving to 3.2 as a baseline.

[pablobm]

Trying to debug this. If I use activerecord-postgis-adapter, with this very same code, I get this warning and error:

$ bin/rails test test/models/geoblock_zone_test.rb
Running 1 tests in a single process (parallelization threshold is 50)
Run options: --seed 64084

# Running:

unknown OID 147959: failed to recognize type of 'zone'. It will be treated as String.
E

Error:
GeoblockZoneTest#test_falls_within_any:
TypeError: Cannot visit RGeo::Cartesian::PointImpl
    app/models/geoblock_zone.rb:38:in 'GeoblockZone.falls_within_any?'
    test/models/geoblock_zone_test.rb:10:in 'GeoblockZoneTest#test_falls_within_any'

However if I don't have this problem in a new Rails app, with same gem versions, model, migration, and test.

So there's something in our gems or config that is interfering with activerecord-postgis-adapter but has no issues with activerecord-postgis.

[pablobm]

Trying to bisect this a bit. It's tricky. For now I know that revision aad81eb shows the same behaviour. I can't compare it with the alternative gem because it requires Ruby 3.3.0, which I can't get working there.

[pablobm]

The last issue was probably me. I think I had forgotten at one point to set adapter: postgis. Summary:

• activerecord-postgis-adapter: "classic" gem, 15 years old. Requires setting adapter: postgis which as I just illustrated can be a headache. Doesn't work well when using geography-enabled functions such as ST_Covers. Does work well with ST_Contains. Works with Ruby 3.2, probably lower but I haven't checked.
• activerecord-postgis: very recent gem, 1 year old, only discovered it by accident. Doesn't require changing config/database.yml. Works with geography-enabled functions such as ST_Covers interpolating ?, but not with Arel (can be added though). Requires Ruby 3.3 minimum (although as mentioned Ruby 3.2 is about to enter EOL). Claims not to be based on monkeypatching in comparison to the competition (I haven't looked into this).

My preference is for the newer gem, if only because of the database.yml thing that does bother me, but I appreciate is only a short term thing. I'm sensing that dropping Ruby 3.2 would be on the cards for maintainers?

[pablobm]

For posterity: the Ruby version question is now moot as this project has changed the minimum to be Ruby 3.3.

[gravitystorm]

We want to get geography vs geometry sorted out here, and this is something I'd like feedback from other people on.

I lean slightly towards geometry, for both theoretical and practical reasons. For example, let's say we draw a rectangular geoblock_zone, with four corners and a top corners at 70°N. For a geometry, the top edge is along the 70°N parallel. But for a geography, the edges of shapes are defined by great-circle arcs between the corners, and this might be unexpected behaviour. Halfway between the top corners, the arc could include 72°N, for example, or go over the pole if the corners are 180° longitude apart. Is this way moderators and mappers would expect?

We (generally?) treat OSM coordinates as a rectangular grid, and ways are (typically?) drawn as straight lines on a plane, rather than great-circle arcs.

So geography is a valid option, but I'm not sure if the extra complexity is worth it. One example of complexity is that many fewer PostGIS functions are implemented for geography compared to geometry.

[pnorman]

We want geometry to have behavior consistent with the rest of the API

[pablobm]

Any preference of factory to use? https://github.com/rgeo/rgeo/blob/main/doc/Which-factory-should-I-use.md I am using RGeo::Cartesian.simple_factory for the moment as it appears to "just work". If we changed to one of the others, we'd need the gem ffi-geos.

[pablobm]

Forgot to mention: I have changed this to be geometry.

[pablobm]

Any ideas on naming? If the current geoblock_zones is not good, options could be: zone_blocks, protected_zones. Also areas instead of zones. Thoughts?

I'm leaning towards protected_zones as "block" could be understood as a noun and cause confusion due to its other meanings.

[gravitystorm]

Also areas instead of zones

Probably not areas, since they have a meaning already in OSM. protected_area also has a specific meaning.

[mmd-osm]

On the proposed API 0.7 page, someone called them "lock regions". I think that's not too bad. The other proposal "Quarantine" sounds a bit too strong for what we want to achieve here.

https://wiki.openstreetmap.org/wiki/API_v0.7#New_Features

[pablobm]

"lock regions". I think that's not too bad

"Locked regions" (if I may change the grammar slightly) could work. Having said that: even if the feature proposed in the wiki may never exist, I'm conscious that we might be overloading the term. I'm thinking whether the concept could be used for both this and what's described in the wiki and I think yes it could, but I could be missing something.

[simonpoole]

May I suggest that doing this with postgis would only make sense if you are considering arbitrary boundaries. If you are just considering blocking at a country/state level there are a number of super fast geocoders (including one used by iD) that do this without all the complexity.

I would note that I'm very much for moving to postgis for other functionality, but for blocking notes it seems a tiny bit of overkill I might say.

[1ec5]

I think the discussions that led us here were focused on arbitrary regions, or at least proximity to a certain location. Blocking an administrative boundary per se might be of less interest for notes specifically, but I could see that being potentially relevant for other kinds of blocking.

super fast geocoders (including one used by iD)

To elaborate, iD uses country-coder, which stores simplified country boundaries in a which-polygon spatial index, which in turn is based on RBush, which is an R-tree implementation, like PostGIS.

[simonpoole]

To elaborate, iD uses country-coder, which stores simplified country boundaries in a which-polygon spatial index, which in turn is based on RBush, which is an R-tree implementation, like PostGIS.

I wasn't implying that the iD implementation was the best in class (it's the worst), just that it isn't a novel idea even for OSM central infrastructure.

[pablobm]

only make sense if you are considering arbitrary boundaries

It's for arbitrary boundaries indeed. For the sake of simplicity and separation of concerns, this PR doesn't include a tool to edit the boundaries, but the plan is to add one at a later stage.

Additionally, adding PostGIS support now would break a psychological barrier (if you will) that will encourage the creation of features that benefit from it.

[pablobm]

Another potential name: moderation zones.

[pablobm]

There's at least the question of naming open, but I think this advanced enough now to be out of draft status.

[pablobm]

I have renamed the model to ModerationZone and tweaked a couple of things.

[pnorman]

I know a comment of this nature is best done early into the review process, but I've been looking over this issue and #6570 and can't find where PostGIS was decided as the best way to do this. Are we satisfied that it's the best option?

[pablobm]

Still better asked now than after merge! 😄

My view is that we are introducing PostGIS here as a strategic move. This PR adds an ancillary feature that can serve as a first, localised use case for PostGIS without affecting OSM at large.

From conversations, I understand that over the years features have been proposed that either required PostGIS or have received a sub-par implementation due to the lack of it. Introducing PostGIS at last breaks a psychological barrier that will allow the creation of new and more performant features in the future.

[pablobm]

There's also the question of how it impacts developers and third party users of the code.

This is an interesting point. I have not had issues in any of the environments I've tried, but I understand there will be use cases that escape me.

One specific case I'm wondering about: @hlfan, I believe you work on Windows? Would you be able to test this PR and tell us if you had any issues getting PostGIS working?

@tomhughes - Are there any specific use cases that you have in mind?

[pablobm]

I see there's been some movement on the operations side (eg: openstreetmap/chef#839). Does the team have an idea of what the current blockers are?

[pablobm]

As of #6994 the minimum Ruby in this project is v3.3, so I have rebased&updated this PR to drop the custom branch that allowed for compatibility of activerecord-postgis with Ruby 3.2 .

[gravitystorm]

I don't have any blockers, I'm happy to merge this unless someone else spots something.

Minor comment about package names inline.

[gravitystorm]

I want to just quibble slightly over the package that we want to install:

• postgis - this only contains client-side stuff, i.e. "PostGIS userland binaries for importing and
  exporting shape and raster files: pgsql2shp, raster2pgsql, and shp2pgsql." We don't need these. However, it also "recommends" the metapackage postgresql-postgis, which is more useful...
• postgresql-postgis is a metapackage that points at whichever postgresql-X-postgis-Y is default on the distribution
• postgresql-X-postgis-Y contains the actual server-side extension (and depends on some other minor packages).

In some places we're installing postgis but relying on (hoping?) the "recommends" packages get installed too. In other places we're installing the postgresql-X-postgis-Y package explicitly, but that will need to kept in sync whenever the underlying distro/image is updated.

Is it preferable to standardise on the postgresql-postgis package throughout?

[seuros]

The question i have, why do you need postgis here ? This is a linting workflow.

[tomhughes]

Because that linter is the database consistency check, so it needs to run the migrations to build the database, which means it needs postgis.

[gravitystorm]

It's in the database_consistency linter, which needs to compare the code against what's in the database.

[pablobm]

OK, so I had a look and this gets a bit tricky...

• The Debian package postgresql-postgis assumes the Postgres version that would be default in the distro. So for example, Debian 13 defaults to Postgres 17. Therefore postgresql-postgis will install postgresql-17-postgis-3, even if we have postgresql-15 installed.
• The Docker images postgres:14, etc, install the requested version of Postgres from source, on Debian. If we then install postgresql-postgis, it will install the wrong flavour of Postgis, or in the wrong library dir or whatever. Doesn't matter, doesn't work.
• In the lint and tests CI workflows we can use postgresql-postgis because they use the default Postgres... which I can't tell where it exactly comes from, but whatever.
• Same for MANUAL_INSTALL.md, where at least I can see where Postgres comes from. For the Fedora instructions, I'm leaving postgis as that appears to the simplest option there.
• For the Dev Container, we are using Postgres 15 (because reasons) so that requires explicitly stating postgresql-15-postgis-3. Similar in the "normal" Docker setup, with postgresql-14-postgis-3.

[gravitystorm]

• The Docker images postgres:14, etc, install the requested version of Postgres from source, on Debian. If we then install postgresql-postgis, it will install the wrong flavour of Postgis, or in the wrong library dir or whatever. Doesn't matter, doesn't work.

OK, thanks for looking into this. I assumed that on postgres:14 the package postgresql-postgis would depend on postgres-14-postgis-3 but from the sound of it, that's not necessarily true and the postgresql-postgis package might point at something else. I'm not sure that it's the most helpful thing from whoever makes those images, but hey-ho - we can deal with it ourselves by being specific where we need to.

[seuros]

The question i have, why do you need postgis here ? This is a linting workflow.

[tomhughes]

I'm a bit confused about the intended scope of "moderation zones" to be honest... I hadn't looked at this in much detail before but my understanding was that this was mostly creating a framework to define zones where various activities could be restricted, a bit like the existing ACL model but for geographic areas.

It appears that despite the generic name these zones are in fact tied explicitly to notes, so should they have a more explicit name? or alternatively something like a type field that indicates what is being moderated and then the note moderation is only applied to zones of the right type, or maybe even a separate one-many table that defines what restrictions are applied to each zone?

Other than that my main comments are that the commit structure doesn't look very good - quite a lot of the last three commits probably needs to be merged into the earlier commit where postgis is added to the database I think so that we don't have commits where, for example, lint can't possibly work.

I'd also question the split with most of the model being introduced in one commit and then the belongs_to rules being added in the next one and would move those into the first commit.

Finally "proof of concept" is a horribly generic commit message that doesn't tell us what it's doing at all.

[tomhughes]

Because that linter is the database consistency check, so it needs to run the migrations to build the database, which means it needs postgis.

[gravitystorm]

It appears that despite the generic name these zones are in fact tied explicitly to notes, so should they have a more explicit name?

I would say that they are being implemented for notes first, with the intention that when that is fully implemented we can add other types in future e.g. changesets.

alternatively something like a type field that indicates what is being moderated and then the note moderation is only applied to zones of the right type, or maybe even a separate one-many table that defines what restrictions are applied to each zone?

Until now I haven't considered having that level of selectivity. I assumed that a zone would apply to both notes and also whatever else is implemented at that stage. I think we can implement what we have (i.e. a zone applies to everything (currently only notes)) and revisit it if moderators need more fine-grained controls in future.

Other than that my main comments are that the commit structure doesn't look very good

That's a good point.

[pablobm]

something like a type field that indicates what is being moderated (...) or maybe even a separate one-many table that defines what restrictions are applied to each zone

This crossed my mind, but I think it's premature. When 🤞 we extend moderation zones to new types of data, then we can see how we handle that. I don't see a reason to do it now.

the commit structure doesn't look very good

I'll have a look.

[pablobm]

Commits should now be better structured 🗂️

[tomhughes]

Until now I haven't considered having that level of selectivity. I assumed that a zone would apply to both notes and also whatever else is implemented at that stage. I think we can implement what we have (i.e. a zone applies to everything (currently only notes)) and revisit it if moderators need more fine-grained controls in future.

I'd accept that argument more if what we had here applied to all notes, but it doesn't, it only applies to notes by people that aren't logged in.

So are you saying that if we extended it to map edits then it would also have to apply to non-anonymous notes at that point, or that it would to anonymous notes but to all map edits which seems very confusing to me.

[tomhughes]

The structure of this looks good now, and my only remaining concerns are around the naming and scope of the zones as discussed.

As that's something that can be resolved as and when we want to expand the use of the zones to other things I think this is good to merge now - thanks for all the work on it.