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

Eas 1893 #27

Closed
wants to merge 151 commits into from
Closed

Eas 1893 #27

wants to merge 151 commits into from

Conversation

aliner-wang
Copy link
Contributor

into_analyzable_function method for review. working on main.rs.

awang7 added 7 commits September 11, 2023 15:01
…nned. And updated alternate_functions to track using entrypoints struct. Todo: capture additional entrypoints for user invokable functions(like macros), write tests + debug, and change main.rs to work with new data struct
… in main.rs and test into_analyzble_function use case in main.rs
@atlassian-cla-bot
Copy link

atlassian-cla-bot bot commented Sep 18, 2023

Thank you for your submission! Like many open source projects, we ask that you sign our CLA (Contributor License Agreement) before we can accept your contribution.
If your email is listed below, please ensure that you sign the CLA with the same email address.

The following users still need to sign our CLA:
[email protected]

Already signed the CLA? To re-check, try refreshing the page.

awang7 added 3 commits September 18, 2023 11:09
// Struct for Custom field Module. Check that search suggestion gets read in correctly.
#[derive(Debug, Clone, PartialEq, Eq, Deserialize)]
pub struct CustomField<'a> {
#[serde(flatten, borrow)]
Copy link
Contributor

Choose a reason for hiding this comment

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

These attributes are unnecessary, since serde automatically borrows from &str.

search_suggestion: &'a str,
function: &'a str,
edit: &'a str,
resolver: ModInfo<'a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resolver: ModInfo<'a>
#[serde(flatten, borrow)]
resolver: Option<ModInfo<'a>>

#[serde(flatten, borrow)]
key: &'a str,
// all attributes below involve function calls
value: &'a str,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe these should all be wrapped in Options, since they aren't required properties in the schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how do you know whether a property is optional or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scratch that I can read

Copy link
Contributor Author

@aliner-wang aliner-wang Sep 18, 2023

Choose a reason for hiding this comment

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

I've updated this struct to:

#[derive(Debug, Clone, PartialEq, Eq, Deserialize)]
pub struct CustomField<'a> {
    #[serde(flatten, borrow)]
    key: &'a str,
    // all attributes below involve function calls
    value: Option<&'a str>,
    search_suggestions: &'a str,
    function: Option<&'a str>,
    edit: Option<&'a str>,
    resolver: Option<ModInfo<'a>>,
}

I think only search_suggestions is the required function attribute cuz it requires a function or an expression? Does that count as required?

Copy link
Contributor

Choose a reason for hiding this comment

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


#[derive(Debug, Clone, PartialEq, Eq, Deserialize)]
pub struct WorkflowPostFunction<'a> {
#[serde(flatten, borrow)]
Copy link
Contributor

Choose a reason for hiding this comment

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

These attributes are unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just the serde attributes right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah

#[derive(Debug, Clone, PartialEq, Eq)]
pub enum FunctionTy<T> {
Invokable(T),
WebTrigger(T),
}

// Struct used for tracking what scan a funtion requires.
#[derive(Debug, Clone, PartialEq, Eq, Deserialize)]
pub struct Entrypoints<'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub struct Entrypoints<'a> {
pub struct Entrypoint<'a> {

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this doesn't need to derive Deserialize.

#[derive(Default, Debug, Clone, PartialEq, Eq, Deserialize)]
struct MacroMod<'a> {
key: &'a str,
function: &'a str,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function: &'a str,
#[serde(borrow)]
function: Option<&'a str>,

The function property is optional, since they can just expose the Macro's main entrypoint through a resolver: https://developer.atlassian.com/platform/forge/manifest-reference/modules/macro/#properties.

// self.webtriggers
// .sort_unstable_by_key(|trigger| trigger.function);

// Get all the Triggers and represent them as a new struct thing where "webtrigger" attribute is true
Copy link
Contributor

Choose a reason for hiding this comment

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

This will create a lot of duplicate functions for the downstream analyzers to consume. It might be better to have a BTreeSet or a sorted Vec containing function names that are on user invokable modules. After you create it, iterate through all the functions appearing on the functions property and toggle the invokable flag if it appears in the set of invokable functions. Afterwards, toggle the web_trigger property if it appears as a webtrigger.

#[derive(Default, Debug, Clone, PartialEq, Eq, Deserialize)]
struct ModInfo<'a> {
function: &'a str,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

If you notice, a lot of modules basically contain at least these three properties: key, function, resolver.function, where the function and resolver properties are optional. You could factor this into a common struct like so:

#[derive(Default, Debug, Clone, PartialEq, Eq, Deserialize)]
struct CommonKeys<'a> {
    key: &'a str,
    #[serde(borrow)]
    function: Option<&'a str>,
    #[serde(borrow)]
    resolver: Option<ModInfo<'a>>,
}

and then embed it into the other structs like so:

#[derive(Default, Debug, Clone, PartialEq, Eq, Deserialize)]
struct MacroMod<'a> {
    #[serde(flatten, borrow)]
    common_keys: CommonKeys<'a>,
    ... other keys 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.

This makes sense to me to simplify the implementation, but how would rust know the mapping? To me, setting common_keys looks like setting a struct within a struct. And keys, function, and resolver would be the first level of the struct.

Copy link
Contributor

Choose a reason for hiding this comment

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

The serde(flatten) attribute will basically just treat the struct's keys as being embedded in the parent struct. See https://serde.rs/attr-flatten.html for more details.

@jwong101 jwong101 marked this pull request as ready for review January 3, 2024 17:10
…point struct when needed. TODO: test and toggle function call in main.rs
…point struct when needed. TODO: test and toggle function call in main.rs
… in main.rs and test into_analyzble_function use case in main.rs
…ent {function: str} entry points. Added missing entrypoints on jira:customField and implmented scanning for those
…on, and resolver for less code duplication. Updated into_analayzable method to update values based on functions specified in function mod. TODO: Update rest of non trigger modules to update mapping to functions to scan from function mod
@jwong101 jwong101 closed this Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants