Fix generation

[AlistairB]

Hi,

I've fixed a couple of issues in the 2 commits so far. I'm mostly cargo culting the fixes so it could be the wrong solution, but it seems to make it pass.

The next issue I don't know how to fix:

[206/215] model:videointelligence
 -> Reading /home/bananan/dead/gogol/gen/annex/videointelligence.json
 -> Reading /home/bananan/dead/gogol/gen/model/videointelligence/v1p3beta1/videointelligence-api.json
 -> Successfully parsed 'videointelligence' API definition.
gogol-gen: Error prefixing: GoogleCloudVideointelligenceV1p1beta1_DetectedLandmark
  Fields: ["confidence","point","name"]
  Matches: 
gcvvdl => Just (fromList ["confidence","point","name"])
g => Just (fromList ["feature","alternatives","languageCode","bottom","annotationProgress","rotatedBoundingBox","startTime","categoryEntities","timeOffset","startTimeOffset","inputUri","left","progressPercent","frames","confidence","updateTime","endTimeOffset","version","annotationResults","words","segments","x","right","top","segment","entity","transcript","vertices","y"])
goo => Just (fromList ["tracks","shotAnnotations","languageCode","bottom","annotationProgress","shotLabelAnnotations","startTime","timeOffset","startTimeOffset","error","inputUri","left","shotPresenceLabelAnnotations","frames","objectAnnotations","confidence","endTimeOffset","frameLabelAnnotations","version","endTime","annotationResults","speechTranscriptions","segments","x","logoRecognitionAnnotations","right","segmentPresenceLabelAnnotations","top","segment","entity","word","normalizedBoundingBox","description","entityId","vertices","segmentLabelAnnotations","speakerTag","explicitAnnotation","y","textAnnotations"])
gcvvdlc => Just (fromList ["confidence","point","name"])
gg => Just (fromList ["languageCode","text","timeOffset","confidence","version","segments","description","entityId"])
gooo => Just (fromList ["tracks","languageCode","startTime","timeOffset","frames","confidence","version","endTime","segments","pornographyLikelihood","entity","word","description","entityId","speakerTag"])
gcvvdl1 => Just (fromList ["confidence","point","name"])
g2 => Just (fromList ["confidence","words","transcript"])
goo3 => Just (fromList ["timeOffset","confidence"])
Makefile:22: recipe for target 'gen' failed
make: *** [gen] Error 1

Any ideas?

[madjar]

Hi there!

So, I wanted to regenerate things to use a new features of the API, and I stumbled on the same problem as you (which I re-fixed, instead of checking your changes first). I also have figured out this last problem, which sadly doesn't have one straightforward solution.

The piece of code that is failing

One of job of the generator is to create types for each type in the API, and lenses for each field. Since different type have fields with the same names, it's necessary to disambiguate the lenses. Here, we do this by adding a prefix based on the type.

This is what the getPrefix function does. For that, it calls acronymPrefixes to get a bunch candidates (we try out a lot of different prefixes). Then, we pick one acronym that isn't already used for these fields.

What went wrong here

So, gogol makes a api wrapper that works for every single version of the api. Sadly, it looks like google went over-the-top with the versioning for some of these api. For this problematic one, videointelligence, we have: v1, v1beta2, v1p1beta, v1p2beta1 and v1p3beta1. Multiple of these have variant of DetectedLandmark, called something like GoogleCloudVideointelligenceV1p1beta1_DetectedLandmark.

So when we get to GoogleCloudVideointelligenceV1p1beta1_DetectedLandmark, we probably have already picked prefixed for a few other DetectedLandmark. So none of the tentative prefixes generated by acronymPrefixes works. And we fail.

How to fix this

• The first family of solutions is to change the prefix candidate generation to make sure we don't run out. The issue with that is it may end up changing the prefix chosen for some existing types, which would result in backward-incompatible changes. I don't know how much that a constraint.
  • The easier way is to go is is to make the last resort fallback even more fallbacky: https://github.com/brendanhay/gogol/compare/develop...madjar:more-prefix-candidates?expand=1. Here, we take all the possible prefix, and try them with all the possible numbers (which are infinite, so it's bound to work). It's ugly, but it works.
  • We could be fancy and try to take into account the version number of the api (eg V1p1beta1) in the prefix.
• Or, more radically, we could try another solution to the duplicated lenses issue, such as "classy" lenses (where you have a typeclass like for each attribute; eg HasName with a lens name). Or any other solution to "the record problem" (which is widely documented).
• Finally, we could decide to drop some of the versions (though I don't know how we would choose).

[AlistairB]

Thanks @madjar for the awesome analysis on the videointelligence issue! ❤️

Solution for right now

I think your solution is perfect as a quick fix.

I'm not sure if it is the right end game solution, but it is better than exploding as it does now and it doesn't introduce any breakage.

Longer term solution

Agree this is tricky. Versions do appear to be deprecated officially ie. https://cloud.google.com/video-intelligence/docs/reference/rest , so perhaps such versions can be dropped from support. I believe v1 is the only version officially supported for videointelligence

Perhaps @brendanhay has plans around this for gogol v2.

Regardless, I think the short term fix is good and essentially a bug fix to the current design. I would create a separate issue for a redesign and prioritize it against other improvements.

---

Anyway, @brendanhay I think is ready for review.

[brendanhay]

Thanks @madjar and @AlistairB!

Longer term (v2) I'll probably just supply the Generic instances and use DuplicateRecordFields - directing users of the libraries to generic-{lens,optics} or RecordDotSyntax when it becomes available. This would "fix" most of the the fields' issues, with all datatypes/constructors being prefixed by version in a consistent fashion.

Regarding:

Finally, we could decide to drop some of the versions (though I don't know how we would choose).

I think we'd probably just keep anything stable with no prerelease suffix, such as v1, v2, etc. and then order any bleeding edge versions and take only the "newest".

For example the available versions [v1, v1beta2, v1p1beta, v1p2beta1, v1p3beta1, v2, v2alpha1, v2alpha3] would only result in [v1, v1beta2, v2, v2alpha3] being considered as inputs for generation.

[madjar]

Awesome! Thanks folks!

I was looking quickly generic lenses and optics, and at RecordDotSyntax to see if there is a quick way to implement it as of now. It would probably be possible to expose the proper typeclass instances to expose lenses and fields, but probably not in a way that works for all approaches without depending on them all. For that, having records that have their actual name, and Generic, will be the way to go.

As a side note, I think RecordDotSyntax will not be enough: given the amount of _Just lenses I had to sprinkle over my read path, lenses are going to be needed.