Trace import: remove activerecord-import gem

[mmd-osm]

This PR removes the activerecord-import dependency and changes the tracepoint import to use import_all! instead.

I hope I didn't miss anything here. In particular Tracepoint.insert_all!(tracepoints.map(&:attributes)) would need a review if that's the proper way to import Tracepoint instances.

Fixes #4994

[tomhughes]

Overall the big difference between import! and insert_all! is that the former runs validations and the latter does not so maybe if we're going to change we should run validate! on each point after it is created?

Really the ideal way to use insert_all! would be not to build the models at all and just build a list of hashes but as it stands we need to run the save callbacks to fill in the tile number...

[tomhughes]

Why do we need to exclude this?

[mmd-osm]

Rubocop was complaining about the use of insert_all! because it's bypassing validations.

[tomhughes]

The main reason I haven't merged this is that I can't make up my mind if I like it... I understand that in theory moving to the built-in version is good but then it turned out they're not equivalent really so we have to jump through hoops and disable cops and it just starts to smell a little.

[mmd-osm]

Well, we could disable rubocop's skip model validation check for that single line only, in case this would be an improvement.

 Tracepoint.insert_all!(tracepoints.map(&:attributes))   # rubocop:disable Rails::SkipsModelValidations

[AntonKhorev]

Do we even need to build Tracepoint models? The only nontrivial thing there is tile calculation.

[mmd-osm]

The question is what happens with validations. Do we want to maintain them in two different places (Tracepoint model and trace.rb)?

[AntonKhorev]

Tracepoint model doesn't need validations because nothing is written using that model.

[mmd-osm]

Tracepoint.import! calls model validations internally, like we discussed above. Try to upload data without timestamp and see what happens.

[AntonKhorev]

Don't insert data without timestamps.

delete_all is already present in trace.rb. It doesn't call any hooks yet nobody is bothered by it. Might as well have insert_all and do all validations in trace.rb.

[mmd-osm]

My goal was to replace the activerecord-import gem by a built in equivalent method. Since the validations are not automatically executed anymore, I added the call to the validate method. Rubocop is not clever enough to figure out that we're validating the model contents earlier on, so we need an additional annotation. I really don't have an issue with that.

Now we could of course further change the implementation and get rid of the model instances. However, that's a second step, and not needed to replace activerecord-import. That's not to say that this isn't a valid discussion to have, it just doesn't fit well here.

[AntonKhorev]

This PR wasn't merged because

we have to jump through hoops and disable cops

and I'm saying that we shouldn't worry here about model validations and what Rubocop thinks about them.

[mmd-osm]

This comment still refers to the previous version where the whole file was added to .rubocop.yml.

@tomhughes hasn’t commented on the inline annotation variant yet, and I would suggest to wait for feedback before continuing.

Like I said, I don’t really have an issue with this annotation. Rubocop is reporting a false positive here and we mark it as such. There's no need to come up with workarounds simply to please the tool.

[AntonKhorev]

Rubocop is reporting a false positive here and we mark it as such.

Any use of insert_all etc that you want to keep is a false positive. You'll have to disable Rubocop for it one way or another.

[mmd-osm]

A few years back, I've introduced both the activerecord-import gem and import! method call to speed up Tracepoint imports. Alternative options weren't available then. Now with the recent Rails version, it's a good time to move to the core Rails code.

At least today, we have exactly one location where insert_all will be used. Nobody else was doing anything with that gem over the past few years. The only other attempt to use activerecord-import was a performance improved changeset upload implementation which never got merged. So it's somewhat unlikely that we will see this issue in many other places in the future.