Replace Jison implementation for PathTemplates by michaelbausor · Pull Request #203 · googleapis/gax-php

michaelbausor · 2018-08-09T22:26:48Z

Important changes in this PR:

Removes Jison (YAY!)
Adds new implementation for resource templates under the ResourceTemplate directory
- The new implementation makes a hard distinction between absolute resource templates (must have leading slash, may have verb) and relative resource templates (never leading slash, never verb)
PathTemplate is refactored to use this new implementation, but retains the ability to contain either an absolute or relative template
Makes the following breaking changes in PathTemplate:
- Removes count method and no longer implements countable interface
- Retains leading '/' if it was present in the original $path parsed (previously this was dropped)
- No longer support verbs that contain '/'
Minor changes to PathTemplate
- Exception messages
- String representation of * no longer changed to {$0=*}
Makes ValidationException a RuntimeException
Adds lots of test cases

When reviewing, please pay close attention to PathTemplateTest, to make sure that there are no regressions in PathTemplate behaviour, other than the expected ones.

Remove PathTemplate

- Restore PathTemplate - Add Parser - Add better test coverage

codecov-io · 2018-08-09T22:38:58Z

Codecov Report

Merging #203 into master will increase coverage by 0.55%.
The diff coverage is 93.7%.

@@            Coverage Diff            @@
##           master    #203      +/-   ##
=========================================
+ Coverage   84.14%   84.7%   +0.55%     
=========================================
  Files          63      63              
  Lines        2826    2353     -473     
=========================================
- Hits         2378    1993     -385     
+ Misses        448     360      -88

Impacted Files	Coverage Δ
src/ApiCore/RequestBuilder.php	`93.6% <100%> (ø)`	⬆️
...Core/ResourceTemplate/RelativeResourceTemplate.php	`100% <100%> (ø)`
src/ApiCore/ResourceTemplate/Segment.php	`82.22% <82.22%> (ø)`
src/ApiCore/PathTemplate.php	`86.66% <83.33%> (-12.35%)`	⬇️
src/ApiCore/ResourceTemplate/Parser.php	`91.42% <91.42%> (ø)`
...Core/ResourceTemplate/AbsoluteResourceTemplate.php	`97.14% <97.14%> (ø)`
src/ApiCore/Serializer.php	`91.89% <0%> (-1.05%)`	⬇️
src/ApiCore/GapicClientTrait.php	`88.88% <0%> (-0.44%)`	⬇️
src/ApiCore/Middleware/AgentHeaderMiddleware.php
src/ApiCore/AgentHeaderDescriptor.php
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4097cbe...d5b1332. Read the comment docs.

tmatsuo

LGTM with minor nits

src/ApiCore/ResourceTemplate/AbsoluteResourceTemplate.php

@@ -0,0 +1,143 @@
+<?php
+/*
+ * Copyright 2018, Google Inc.


src/ApiCore/ResourceTemplate/Segment.php

+    public function __toString()
+    {
+        if ($this->hasValue()) {
+            return $this->value;


src/ApiCore/ValidationException.php

 * ValidationException represents a local error (i.e. not during an RPC call).
 */
-class ValidationException extends Exception
+class ValidationException extends RuntimeException


src/ApiCore/ResourceTemplate/Parser.php

+
+    public static function isValidBinding($literal)
+    {
+        return preg_match("-^[^/=\\{\\}\\\\]+$-", $literal) === 1;


jdpedrie · 2018-08-10T18:39:43Z

Cloud Firestore supports special characters in paths which are not allowed by the jison parser or by this new parser. I added a workaround (googleapis/google-cloud-php#1122) for this a couple of months ago.

When I made that change, we had been hoping it would be a temporary workaround pending this work we knew you had on your agenda. Would it be possible to add support for optional special characters in paths?

See also #197.

michaelbausor · 2018-08-10T19:04:52Z

@jdpedrie Yes, I think we can add such support. Are there any more detailed requirements? E.g., are there restrictions on the first character of the file name? If so, we would like to enforce them by not allowing the special characters.

jdpedrie · 2018-08-10T19:23:50Z

@michaelbausor I think the only constraints for Firestore are listed under "Constraints on document IDs" found here. My tests confirm that special characters are, for instance, allowed as the first character of an ID.

michaelbausor · 2018-08-10T20:36:48Z

Updated, PTAL

tmatsuo

Still looking good!

jdpedrie

looks good! nice work with the comprehensive tests. :)

src/ApiCore/ValidationException.php

@@ -32,10 +32,11 @@
 namespace Google\ApiCore;

 use Exception;


dwsupplee · 2018-08-15T17:46:12Z

Running through a review today :). Really happy to see this!

dwsupplee

This is a really solid refactor. Nice work :)

src/ApiCore/ResourceTemplate/Parser.php

+    /**
+     * Parses a path into an array of segments.
+     *
+     * @param $path


src/ApiCore/ResourceTemplate/Parser.php

+    private static function parse($segmentString)
+    {
+        if ($segmentString === '*') {
+            return new Segment(Segment::WILDCARD_SEGMENT, null, null);


src/ApiCore/ResourceTemplate/Parser.php

+     */
+    private static function parseLiteralFromPath($literal, $path, &$index)
+    {
+        if (strlen($path) < ($index + strlen($literal))) {


src/ApiCore/ResourceTemplate/Parser.php

+     * @param string $literal
+     * @return bool
+     */
+    public static function isValidLiteral($literal)


src/ApiCore/ResourceTemplate/Parser.php

+     * @param $binding
+     * @return bool
+     */
+    public static function isValidBinding($binding)


src/ApiCore/ResourceTemplate/RelativeResourceTemplate.php

+     * @param Segment[] $segments
+     * @return int
+     */
+    private static function countDoubleWildcards($segments)


src/ApiCore/ResourceTemplate/RelativeResourceTemplate.php

+                    foreach ($nestedKeySegmentTuples as list($nestedKey, $nestedSegment)) {
+                        /** @var Segment $nestedSegment */
+                        // Nested variables are not allowed
+                        assert($nestedSegment->getSegmentType() !== Segment::VARIABLE_SEGMENT);


src/ApiCore/ResourceTemplate/RelativeResourceTemplate.php

+        // match by examining the difference in count between the template segments and the
+        // path pieces.
+
+        if (count($pathPieces) < count($flattenedKeySegmentTuples)) {


src/ApiCore/ResourceTemplate/Segment.php

+    /** @var RelativeResourceTemplate|null */
+    private $template;
+
+    public function __construct($segmentType, $key, $value, RelativeResourceTemplate $template = null)


tests/ApiCore/Tests/Unit/PathTemplateTest.php

+    /**
+     * @expectedException \Google\ApiCore\ValidationException
+     */
    public function testMatchWildcardWithColonInMiddle()


michaelbausor added 8 commits August 2, 2018 10:53

Initial work to remove jison

e006535

Add ResourceTemplate and AbsoluteResourceTemplate

4e54a1a

Remove PathTemplate

Remove Jison

decf5d6

Update templates and segment

81bd6d3

Restore check when binding value to variable segment

a06382e

Major update to PathTemplate

749869d

- Restore PathTemplate - Add Parser - Add better test coverage

Cleanup

11fd434

Revert comment changes

fdeb3bc

michaelbausor requested review from bshaffer, dwsupplee, jdpedrie and tmatsuo August 9, 2018 22:26

Update tests, remove unused

03e4bde

tmatsuo approved these changes Aug 10, 2018

View reviewed changes

Address PR comments and cleanup docs

635be4e

Address issue googleapis#197

360a0b3

tmatsuo approved these changes Aug 15, 2018

View reviewed changes

jdpedrie reviewed Aug 15, 2018

View reviewed changes

src/ApiCore/ValidationException.php

@@ -32,10 +32,11 @@

namespace Google\ApiCore;

use Exception;

This comment was marked as spam.

Sign in to view

Remove unused import

eb61b1c

Revert ValidationException to extend Exception

d5b1332

michaelbausor merged commit 1c74d0f into googleapis:master Aug 16, 2018

michaelbausor deleted the path-templates branch August 16, 2018 16:31

dwsupplee reviewed Aug 16, 2018

View reviewed changes

michaelbausor mentioned this pull request Aug 20, 2018

Prepare 0.37.0 #211

Merged

Conversation

michaelbausor commented Aug 9, 2018

Uh oh!

codecov-io commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tmatsuo left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

jdpedrie commented Aug 10, 2018

Uh oh!

michaelbausor commented Aug 10, 2018

Uh oh!

jdpedrie commented Aug 10, 2018

Uh oh!

michaelbausor commented Aug 10, 2018

Uh oh!

tmatsuo left a comment

Choose a reason for hiding this comment

Uh oh!

jdpedrie left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

dwsupplee commented Aug 15, 2018

Uh oh!

dwsupplee left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

codecov-io commented Aug 9, 2018 •

edited

Loading