Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Conversation

@boydc2014
Copy link
Contributor

@boydc2014 boydc2014 commented Sep 4, 2019

Description

Resource

Resource is a new interface created in this PR, which is a higher level abstraction of all declarative assets (dialog, lg, lu, schema, etc).

Motivation

Resource reference resolving

The background is adpative SDK used a "ResourceExplorer" which take over the resource management, including many aspects. Some aspects like basic CRUD, which is previously done in composer side with direct disk r/w. But other aspects like resource reference, which leverage a different way to identify resources (other than relative file path) and the interpretation\resolving of this id system.

For example, the SDK resource explorer would enable you to not specifying a full path when referring a lg file, you can just give the name, no matter you are referring a lg file from a dialog file, or from another lg file. Which means resource explorer takes over the reference resolving between dialog <-> lg, and between lg <-> lg. (side story, resource explorer today, don't handle lu<->lu reference)

For our validation scenario, the very first principle is to make sure, the validation works the same way as the runtime, which requires us to also create an abstraction layer enabling us to mimic the way how resource is identified, and how those reference are being interpreted in runtime.

Resource management

It might be debatable of whether it's reasonable to allow another approach of interpreting the resource reference, rather than just use relative path like many other systems do. Even we forget about the resource solving issue, bringing an abstraction for declarative assets also make the resource management significant easier.

We can share more common logic in this layer, and reduce lots of duplication\inconsistency, and the system can be more stable and predictable. Just in this PR, I spotted several inconsistency of handling difference kind of resources. A shared layer for basic resource management can bring us further.

Managed Lu

It also benefits managed lu scenario in certain level, the managed LU story requires the capability of manipulating lu content, with this resource interface, we move the generic functionality into resource, specific features for each type resource can be captured naturally inside each resource.

Changes summary

Major

  • Create unified 'Resource' interface, move the old indexer capability inside each resource, so that whenever the resource is resolved, it will return with fully-indexer information.
  • Replace the
  • Create ResourceResolver interface, which is generic interface we want LG\LU parser to adapt to.
  • Create a unified Diagnostic interface to support validation info from different source, largely borrowed from vscode, with our own requirement of json path location.
  • Combine Resource + ResourceResolver + Diagnostic, create a ResourceValidator interface, and create different validators for different type of resource.
  • Validate all resources after any resource change

Minor

  • clean up lupublisher interface
  • allowing invalid lu content to be saved, validate should report that instead of blocking saving
  • leave many notes of places i see inconsistent

Note, this PR is focusing on the abstraction, the interface, the mechanism, not any specific validation work, so i mark this as a code factor work.

Next

  • Implement each validator more thoroughly
  • Hook up with UX

Task Item

closes #694

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code refactor (non-breaking change which improve code quality, clean up, add tests, etc)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Doc update (document update)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have functionally tested my change

@boydc2014 boydc2014 marked this pull request as ready for review September 27, 2019 02:33
Copy link
Member

@cwhitten cwhitten left a comment

Choose a reason for hiding this comment

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

Good start, I'd like the see the following:

  1. Some more comments as to how the public API will be exposed and used in the application
  2. How we can leverage this system in custom schema scenarios
  3. The convergence plan for the dialogLint tool

case 'Microsoft.SendActivity':
target = value.activity;
break;
case 'Microsoft.TextInput':
Copy link
Member

Choose a reason for hiding this comment

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

This switch feels hard-coded, and this the only prompt where we hook in to LU, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This switich is hard-coded to extract lg template from certain type of actions. We only SendActivity and TextInput leverage lg template now.

This part is just moving the previous extracting logic into dialogResource. not adding\removing any features.

id: `events[${index}]`,
displayName: '',
type: rule.$type,
isIntent: rule.$type === 'Microsoft.OnIntent',
Copy link
Member

Choose a reason for hiding this comment

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

Microsoft.OnIntent should come from the shared-menus package, not hard-coded here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as stated above, here i just put the code pieces into the right location, not intended to change it's behavior. And yeah, i will put a comments here, as i do in other places when i spot something not promising.

* This is the one single validation library for any declartive assets
* that can power both command-line tool and composer
*/
export class DeclartiveValidator implements ResourceValidator {
Copy link
Member

Choose a reason for hiding this comment

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

s/DeclartiveValidator/DeclarativeValidator

}

if (validator !== null) {
return validator.validate(resource, resolver);
Copy link
Member

Choose a reason for hiding this comment

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

is this where all the validation will happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, and this DeclativeValidator is actually the expected entry of "dialogLint".

See comments below how i vision a, how to handle customized schema, b. how to converge dialogLint


/**
* TODO:
* 1. use schema to verify json structure
Copy link
Member

Choose a reason for hiding this comment

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

the schema used to do the validate needs to from the caller. as in any bot with it's own schema needs to be able to use this system of classes and validation will just work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the design here is schema is located and loaded into resourceExplorer, and when valiadtor needs it, it ask resourceResolver.

It's behaved the same way like any other resources needed to validate a dialog.

For example, in a.dialog,
{
$schema: "path to schema",
generator": "file name of lg file",
recognizer: "file name of lu file",
...
}

to validate such a dialog, it depends the actual content from schema file, lg file, lu file, and those files are resolved in a consistent way, by asking the resolver to get the resource content.

So, no matter it's customized schema or official schema release by us, it will use "$schema" to identify the source, and resource resolver will be able to resolve that. That's also why there is a "SchemaResource"

Sounds good?

@boydc2014
Copy link
Contributor Author

  1. How public api will be exposed?

Although there is many validators for each resource, there is only one entry to this whole validation functionality which is "DeclarativeValidator" which is also going to be the only public facing API surfaced out in Shell API, as also surfaced out as a library when we move this out.

  1. How customized schema is handled?
    See comments above Resource/Validator interface #803 (comment)

  2. How to coverage with dialogLint?
    As stated in some comments and also in Update README.md #1, the plan is we merged the functionality of dialogLint into dialogValidator. And we move out the entire "resource" and "validator" folder out as one (or two) new library, the DeclartiveValidator will be the "enhanced" DialogLint.

Quick recap of the previous gap we see in DialogLint. DialogLint does dialog validation better than us now(which leveraged the schema), so we want that, but DialogLint don't handle LG\LU, so we create LG\LU to fill that gap. DialogLint coupled some file operation, we replaced that totally with the new resource system decoupled from storage location, which aligns better with the direction of runtime.

@boydc2014 boydc2014 mentioned this pull request Oct 11, 2019
5 tasks
@boydc2014
Copy link
Contributor Author

Close this for now, i will rework on this, since some significant change in architecture would happen in master

@boydc2014 boydc2014 closed this Oct 23, 2019
@cwhitten cwhitten deleted the donglei/validation-interface branch October 23, 2019 17:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Linting & Validation

3 participants