Issue #571548: Prevent Views generating SQL aliases which are too-long or duplicates

Closes #571548

Use safe SQL aliases.

We limit the length of the original alias (to a default maximum of 60 characters), incorporating a hash of the original value for uniqueness if that value was too long to use verbatim.

This prevents subsequent truncation from creating duplicate aliases in cases where two or more aliases are identical up to the point of truncation. This happens particularly easily with relationships, where alias names may be built from several concatenated identifiers.

changed title from Issue #571548: Prevent Views generating SQL aliases longer than 60 chars to Issue #571548: Prevent Views generating SQL aliases which are too-long or duplicates

changed the description

added 1 commit

• 7a1ef666 - Add type-hinting for the $maxlength argument

Compare with previous version

resolved all threads

added 1 commit

• 2e661f89 - fixup! Add type-hinting for the $maxlength argument

Compare with previous version

marked this merge request as draft from issue/drupal-571548@2e661f89

added 1 commit

• 4a26e63e - Add type-hinting for the $maxlength argument

Compare with previous version

marked this merge request as ready

added 1 commit

• c9ca7d27 - Add type-hinting for the $maxlength argument

Compare with previous version

added 1 commit

• efb734a5 - Add mixed type-hinting for the $alias argument

Compare with previous version

Why use crc32 which returns an int? In core, it is more common to use md5.

Because it produces short hashes, meaning that we don't need to truncate the result as an additional step (which was going to be necessary with every other option I looked at).

I think my expectation was also that a function which was designed to return a shorter hash might well be less likely to produce a collision than truncation of longer hashes (which might turn out to have common prefixes). I don't know whether that's actually the case, though (and I imagine the chances are vanishingly small in either case, so I feel like this concern is probably only theoretical).

I didn't see an obvious reason not to go with crc32 here, but I'll happily defer to someone who understands the maths better than myself.

Potentially nothing more than an aesthetic choice, but I no longer remember.

In fact the lower-case-ness was me being consistent with the old code, which did this:

    // PostgreSQL truncates aliases to 63 characters:
    // https://www.drupal.org/node/571548.

    // We limit the length of the original alias up to 60 characters
    // to get a unique alias later if its have duplicates
    $alias = strtolower(substr($alias, 0, 60));

So as aliases are forced to lower-case in current releases, I think it's sensible to maintain that.

This looks like it is working to make the alias unique. Perhaps this can be removed?

I'd need to examine the code again, but from memory I think that was pre-existing code which protects against the generation of identical aliases which will be used for different purposes.

The sanitizer doesn't help in that situation (its job is not to return a different alias every time, but rather to return a safe version of what it's given -- and always the same output for a given input), so we probably still need this code.

(Of course if those 'uniquified' aliases are too long, the sanitizer will produce a unique shorter alias for each of them.)

Yes, that code is to ensure a NEW relationship alias is generated, so it needs to be different to anything already known.

It does look like there's a bug here, though...

The code does work if we call addRelationship() twice with the same short alias (edit: make that a short lower-case alias): the sanitizer won't modify that alias, and therefore it will exist as a key in $this->relationships the second time we call the function, and we'll notice that and add a unique numeric suffix to make it different.

If we call addRelationship() twice with the same LONG alias (or one with upper-case letters) then we'll end up with a conflict, because when we assign $this->relationships[$alias] a few lines later we've sanitized $alias to a shorter value. So the next time we call addRelationship() with the original/long would-be-duplicate alias it won't find it in $this->relationships, and hence it will end up with the same sanitized value as the first call.

Note that we do still NEED to sanitize the alias AFTER the suffix has been added, because the addition of the suffix might be the thing which pushes the length of the alias over the limit.

If I'm not mis-reading the situation, I suspect we want:

    // Make sure $alias isn't already used.
    $alias_base = $alias;
    $count = 1;
    $alias = $this->sanitizeAlias($alias_base);
    while (!empty($this->relationships[$alias])) {
      $alias = $this->sanitizeAlias($alias_base . '_' . $count++);
    }

Could someone please review this bug/fix, in case I'm missing some reason why the code was actually ok? (E.g. if the un-sanitized alias ends up as a key in that array as well as the sanitized version.)

Can this be more specific? Isn't this just string|null ?

Off the top of my head I'm not 100% certain at this point; I'd need to scour all the code again. The main thing is that it returns any non-string value unchanged.

I think this is good as-is; but if anyone feels motivated to establish with certainty that nothing could possibly cause the argument to be anything other than string|null (or perhaps some extended set of specific types), then that's fine by me.