Support versions in elements multi fetch

[AntonKhorev]

As supported by cgimap. Related to #1189 which is a different implementation with different API entry points.

[mmd-osm]

ElementsApiController seems like a good way to maximize code reuse. However, I find things like current_model = controller_name.classify.constantize and old_model = "Old#{controller_name.classify}".constantize too difficult to read for a low skilled dev. Given that it would only save us a few lines of code, it feels a bit over-engineered to me.

I wouldn't mind to see a bit more code duplication here, for the sake of better code readability.

[AntonKhorev]

Given that it would only save us a few lines of code, it feels a bit over-engineered to me.

With a proper abstract base class, NodesController can be reduced to basically nothing.

things like current_model = controller_name.classify.constantize and old_model = "Old#{controller_name.classify}".constantize too difficult to read

Looks like Rails still doesn't provide anything better than that to get model classes without naming them directly. But if that's really too scary, I can make a couple of methods in child controller classes that return their model classes.

[mmd-osm]

But if that's really too scary, I can make a couple of methods in child controller classes that return their model classes.

I also found The parameter #{controller_name} is required a bit too extreme. Yes, it's super generic, but might be too fragile going forward (think: translations, maybe) . To be honest, I would completely remove the ElementsApiController, and add around 10 lines to each of three controllers. Perfectly fine for me.

Let's wait what both maintainers think about it, before doing any changes.

[AntonKhorev]

I also found The parameter #{controller_name} is required a bit too extreme. Yes, it's super generic, but might be too fragile going forward (think: translations, maybe)

How do you think the translation string would look like? I think it would be about as extreme. And again, I can do abstract method returning the message.

I would completely remove the ElementsApiController

Why not remove ApiController too?

and add around 10 lines to each of three controllers.

I'd remove around 80 lines from each.

I already managed to sneak in an abstract class removing triple copy-paste, although it was in javascript.

[tomhughes]

I was wondering how on earth cgimap wound up with an API extension relative to rails and it it turns out the history is far older and more baroque than I was expecting.

The cgimap version came in with zerebubuth/openstreetmap-cgimap#129 which also added the single object versions calls which rails does have - that claims to be implementing #1189 for cgimap but actually is using a subtly different syntax.

That is probably a rather unfortunate syntax in fact because unlike #1189 it doesn't maintain the separation between main controller methods and history controller methods which then leads to the code here having to a complicated dance with the main controller accessing history objects and also to the non-deterministic uniqification where if you ask for the current version of an object in both versioned and unversioned form you don't know which one will be returned.

Now of course they should always be the same but it's an interesting wart.

I don't see any obvious problem with the code other than the rather complex nature of the abstract parent as @mmd-osm noted which is not ideal but then equally neither is duplicating everything.

Possibly it would better to add new tests rather than add lots of cases to the existing ones. I suspect @gravitystorm will think so though he'd probably also say we should split the existing one.

[AntonKhorev]

Updates:

• moved the generic elements controller into api module; this module already had the generic old elements controller OldController
• put version multifetch tests into different methods
• replaced .classify.constantize with abstract current_model/old_model methods

[AntonKhorev]

Removed controller_name, using type_plural = current_model.model_name.plural instead.