Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(docs): add validation of xrefs to docfx command #6658

Merged
merged 7 commits into from
Jun 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/docs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:
id: extract
uses: shrink/actions-docker-extract@v2
with:
image: "phpdoc/phpdoc:20230522201300af6fb5"
image: "phpdoc/phpdoc:3.4.1"
path: "/opt/phpdoc/."
- name: Symlink phpDocumentor
run: ln -s $(pwd)/${{ steps.extract.outputs.destination }}/bin/phpdoc /usr/local/bin/phpdoc
Expand Down
2 changes: 1 addition & 1 deletion .kokoro/docs/docker/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ RUN php -r "copy('https://getcomposer.org/installer', 'composer-setup.php');" &&
# Use phpDocumentor from HEAD until they create a new release
# TODO: Remove once phpDocumentor tags a new release
# @see https://github.com/phpDocumentor/phpDocumentor/issues/3434
COPY --from=phpdoc/phpdoc:20230522201300af6fb5 /opt/phpdoc /opt/phpdoc
COPY --from=phpdoc/phpdoc:3.4.1 /opt/phpdoc /opt/phpdoc
RUN ln -s /opt/phpdoc/bin/phpdoc /usr/local/bin/phpdoc
ENV PHPDOC_ENV=prod

Expand Down
67 changes: 60 additions & 7 deletions dev/src/Command/DocFxCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,30 @@
use Symfony\Component\Process\Process;
use Symfony\Component\Yaml\Yaml;
use RuntimeException;
use Google\Cloud\Dev\Component;
use Google\Cloud\Dev\DocFx\Node\ClassNode;
use Google\Cloud\Dev\DocFx\Page\PageTree;
use Google\Cloud\Dev\DocFx\Page\OverviewPage;
use Google\Cloud\Dev\Component;
use Google\Cloud\Dev\DocFx\XrefValidationTrait;

/**
* @internal
*/
class DocFxCommand extends Command
{
use XrefValidationTrait;

private array $composerJson;
private array $repoMetadataJson;

// these links are inexplicably broken in phpdoc generation, and will require more investigation
private static array $allowedReferenceFailures = [
'\Google\Cloud\ResourceManager\V3\Client\ProjectsClient::testIamPermissions()'
=> 'ProjectsClient::testIamPermissionsAsync()',
'\Google\Cloud\Logging\V2\Client\ConfigServiceV2Client::getView()'
=> 'ConfigServiceV2Client::getViewAsync()'
];

protected function configure()
{
$this->setName('docfx')
Expand All @@ -50,8 +62,8 @@ protected function configure()
'component-path',
'',
InputOption::VALUE_OPTIONAL,
'Specify the path of the desired component. Please note, this option is only intended for testing purposes.
')
'Specify the path of the desired component. Please note, this option is only intended for testing purposes.'
)
;
}

Expand All @@ -61,7 +73,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
throw new RuntimeException('This command must be run on PHP 8.0 or above');
}

$componentName = $input->getOption('component') ?: basename(getcwd());
$componentName = rtrim($input->getOption('component'), '/') ?: basename(getcwd());
$component = new Component($componentName, $input->getOption('component-path'));
$output->writeln(sprintf('Generating documentation for <options=bold;fg=white>%s</>', $componentName));
$xml = $input->getOption('xml');
Expand Down Expand Up @@ -92,8 +104,10 @@ protected function execute(InputInterface $input, OutputInterface $output)
$indent = 2; // The amount of spaces to use for indentation of nested nodes
$flags = Yaml::DUMP_MULTI_LINE_LITERAL_BLOCK;

$valid = true;
$tocItems = [];
$packageDescription = $component->getDescription();
$isBeta = 'stable' !== $component->getReleaseLevel();
foreach ($component->getNamespaces() as $namespace => $dir) {
$pageTree = new PageTree(
$xml,
Expand All @@ -103,6 +117,8 @@ protected function execute(InputInterface $input, OutputInterface $output)
);

foreach ($pageTree->getPages() as $page) {
// validate the docs page. this will fail the job if it's false
$valid = $this->validate($page->getClassNode(), $output) && $valid;
$docFxArray = ['items' => $page->getItems()];

// Dump the YAML for the class node
Expand All @@ -117,11 +133,15 @@ protected function execute(InputInterface $input, OutputInterface $output)
$tocItems = array_merge($tocItems, $pageTree->getTocItems());
}

$releaseLevel = $component->getReleaseLevel();
// exit early if the docs aren't valid
if (!$valid) {
return 1;
}

if (file_exists($overviewFile = sprintf('%s/README.md', $component->getPath()))) {
$overview = new OverviewPage(
file_get_contents($overviewFile),
$releaseLevel !== 'stable'
$isBeta
);
$outFile = sprintf('%s/%s', $outDir, $overview->getFilename());
file_put_contents($outFile, $overview->getContents());
Expand All @@ -133,7 +153,7 @@ protected function execute(InputInterface $input, OutputInterface $output)
$componentToc = array_filter([
'uid' => $component->getReferenceDocumentationUid(),
'name' => $component->getPackageName(),
'status' => $releaseLevel !== 'stable' ? 'beta' : '',
'status' => $isBeta ? 'beta' : '',
'items' => $tocItems,
]);
$tocYaml = Yaml::dump([$componentToc], $inline, $indent, $flags);
Expand Down Expand Up @@ -199,4 +219,37 @@ public static function getPhpDocCommand(string $componentPath, string $outDir):

return $process;
}

private function validate(ClassNode $class, OutputInterface $output): bool
{
$valid = true;
$emptyRef = '<options=bold>empty</>';
$isGenerated = $class->isProtobufMessageClass() || $class->isProtobufEnumClass() || $class->isServiceClass();
foreach (array_merge([$class], $class->getMethods(), $class->getConstants()) as $node) {
foreach ($this->getInvalidXrefs($node->getContent()) as $invalidRef) {
if (isset(self::$allowedReferenceFailures[$node->getFullname()])
&& self::$allowedReferenceFailures[$node->getFullname()] == $invalidRef) {
// these links are inexplicably broken in phpdoc generation, and will require more investigation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create a ticket for this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a tricky situation because the issue (I think) is in phpdoc itself... but there's no indication AFAIK for why this would happen (they look identical to the other xrefs). It's absolutely baffling.

Before I can file an issue with phpdoc, I need to do more investigation to see if I can find out what could be causing it. Because I truly have no clue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed an issue in #7377

continue;
}
$output->write(sprintf("\n<error>Invalid xref in %s: %s</>", $node->getFullname(), $invalidRef));
$valid = false;
}
foreach ($this->getBrokenXrefs($node->getContent()) as $brokenRef) {
$output->writeln(
sprintf('<comment>Broken xref in %s: %s</>', $node->getFullname(), $brokenRef ?: $emptyRef),
$isGenerated ? OutputInterface::VERBOSITY_VERBOSE : OutputInterface::VERBOSITY_NORMAL
);
// generated classes are allowed to have broken xrefs
if ($isGenerated) {
continue;
}
$valid = false;
}
}
if (!$valid) {
$output->writeln('');
}
return $valid;
}
}
8 changes: 8 additions & 0 deletions dev/src/DocFx/Node/ClassNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@ public function isProtobufEnumClass(): bool
return 0 === strpos($lastDescriptionLine, 'Protobuf type');
}

public function isProtobufMessageClass(): bool
{
if ($extends = $this->getExtends()) {
return '\Google\Protobuf\Internal\Message' === $extends;
}
return false;
}

public function isGapicEnumClass(): bool
{
// returns true if the class extends \Google\Protobuf\Internal\Message
Expand Down
114 changes: 114 additions & 0 deletions dev/src/DocFx/XrefValidationTrait.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
<?php
/**
* Copyright 2024 Google Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

namespace Google\Cloud\Dev\DocFx;

use Google\Cloud\Core\Logger\AppEngineFlexFormatter;
use Google\Cloud\Core\Logger\AppEngineFlexFormatterV2;

/**
* @internal
*/
trait XrefValidationTrait
{
/**
* Verifies that all class references and @see tags are properly formatted
* with either an FQSEN (Fully Qualified Structural Element Name), or a URL.
*/
private function getInvalidXrefs(string $description): array
{
$invalidRefs = [];
preg_replace_callback(
'/<xref uid="([^ ]*)"/',
function ($matches) use (&$invalidRefs) {
// Valid external reference
if (0 === strpos($matches[1], 'http')) {
return;
}
// Invalid reference format (not an FQSEN)
if ('\\' !== $matches[1][0]) {
$invalidRefs[] = $matches[1];
return;
}
// Invalid FQSEN (If it contains "\Google\" more than once, it wasn't properly imported)
if (substr_count($matches[1], '\Google\\') > 1) {
$invalidRefs[] = $matches[1];
return;
}
},
$description
);

return $invalidRefs;
}

/**
* Verifies that all class references and @see tags contain references to classes, methods, and
* constants which actually exist.
*/
private function getBrokenXrefs(string $description): array
{
$brokenRefs = [];
preg_replace_callback(
'/<xref uid="([^ ]*)"/',
function ($matches) use (&$brokenRefs) {
// Valid external reference
if (0 === strpos($matches[1], 'http')) {
return;
}
// We cannot run "class_exists" on these classes because they will throw a fatal error.
if (in_array(
$matches[1],
['\\' . AppEngineFlexFormatter::class, '\\' . AppEngineFlexFormatterV2::class]
)) {
return;
}
// Valid class reference
if (class_exists($matches[1]) || interface_exists($matches[1] || trait_exists($matches[1]))) {
return;
}
// Valid method, magic method, andd constant references
if (false !== strpos($matches[1], '::')) {
if (false !== strpos($matches[1], '()')) {
list($class, $method) = explode('::', str_replace('()', '', $matches[1]));
// Valid method reference
if (method_exists($class, $method)) {
return;
}
// Assume it's a magic Async method
if ('Async' === substr($method, -5)) {
return;
}
} elseif (defined($matches[1])) {
// Valid constant reference
return;
}
}
// Invalid reference!
if ($matches[1] === '\\\\') {
// empty hrefs show up as "\\"
$brokenRefs[] = null;
} else {
$brokenRefs[] = $matches[1];
}
},
$description
);

return $brokenRefs;
}
}
72 changes: 71 additions & 1 deletion dev/tests/Unit/DocFx/NodeTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,24 @@

namespace Google\Cloud\Dev\Tests\Unit\DocFx;

use PHPUnit\Framework\TestCase;
use Google\Cloud\Dev\DocFx\Node\ClassNode;
use Google\Cloud\Dev\DocFx\Node\MethodNode;
use Google\Cloud\Dev\DocFx\Node\XrefTrait;
use Google\Cloud\Dev\DocFx\Node\FencedCodeBlockTrait;
use Google\Cloud\Dev\DocFx\XrefValidationTrait;
use PHPUnit\Framework\TestCase;
use Prophecy\Argument;
use Prophecy\PhpUnit\ProphecyTrait;
use Symfony\Component\Console\Output\OutputInterface;
use SimpleXMLElement;

/**
* @group dev
*/
class NodeTest extends TestCase
{
use ProphecyTrait;

public function testNestedParameters()
{
$nestedParamsXml = file_get_contents(__DIR__ . '/../../fixtures/phpdoc/nestedparams.xml');
Expand Down Expand Up @@ -433,4 +439,68 @@ public function provideStatusAndVersionByNamespace()
['Foo', ''],
];
}

/**
* @dataProvider provideInvalidXrefs
*/
public function testInvalidXrefs(string $description, array $invalidXrefs = [])
{
$classXml = '<class><full_name>TestClass</full_name><docblock><description>%s</description></docblock></class>';
$class = new ClassNode(new SimpleXMLElement(sprintf($classXml, $description)));

$validator = new class () {
use XrefValidationTrait {
getInvalidXrefs as public;
}
};

$this->assertEquals($invalidXrefs, $validator->getInvalidXrefs($class->getContent()));
}

public function provideInvalidXrefs()
{
return [
['{@see \DoesntExist}'], // class doesn't exist, but is still a valid xref
['{@see \SimpleXMLElement::method()}'], // method doesn't exist, but is still a valid xref
['{@see \SimpleXMLElement::addAttribute()}'], // valid method
['{@see \SimpleXMLElement::OUTPUT_FOO}'], // constant doesn't exist, but is still a valid xref
[sprintf('{@see \%s::OUTPUT_NORMAL}', OutputInterface::class)], // valid constant
['Take a look at {@see https://foo.bar} for more.'], // valid
['{@see Foo\Bar}', ['Foo\Bar']], // invalid
['Take a look at {@see Foo\Bar} for more.', ['Foo\Bar']], // invalid
[
'{@see \Google\Cloud\PubSub\Google\Cloud\PubSub\Foo}',
['\Google\Cloud\PubSub\Google\Cloud\PubSub\Foo']
], // invalid
];
}

/**
* @dataProvider provideBrokenXrefs
*/
public function testBrokenXrefs(string $description, array $brokenXrefs = [])
{
$classXml = '<class><full_name>TestClass</full_name><docblock><description>%s</description></docblock></class>';
$class = new ClassNode(new SimpleXMLElement(sprintf($classXml, $description)));

$validator = new class () {
use XrefValidationTrait {
getBrokenXrefs as public;
}
};

$this->assertEquals($brokenXrefs, $validator->getBrokenXrefs($class->getContent()));
}

public function provideBrokenXrefs()
{
return [
['{@see \OutputInterface}', ['\OutputInterface']], // invalid class (doesn't exist)
['{@see \SimpleXMLElement}.'], // valid class
['{@see \SimpleXMLElement::method()}', ['\SimpleXMLElement::method()']], // invalid method (doesn't exist)
['{@see \SimpleXMLElement::addAttribute()}'], // valid method
['{@see \SimpleXMLElement::OUTPUT_FOO}', ['\SimpleXMLElement::OUTPUT_FOO']], // invalid constant (doesn't exist)
[sprintf('{@see \%s::OUTPUT_NORMAL}', OutputInterface::class)], // valid constant
];
}
}
Loading