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

DCS with namespace target, don't apply decorators on top of the namespace. #874

Closed
sanketshevkar opened this issue Jul 8, 2024 · 5 comments · Fixed by #877
Closed

DCS with namespace target, don't apply decorators on top of the namespace. #874

sanketshevkar opened this issue Jul 8, 2024 · 5 comments · Fixed by #877
Assignees

Comments

@sanketshevkar
Copy link
Member

sanketshevkar commented Jul 8, 2024

Bug Report 🐛

The following model definition is valid...

@Term("My HR Model")
@Term_description("Reusable data definitions for the HR department.")
namespace [email protected]

Current Behavior

When we apply decorator using dcs targeting the namespace,

{
        "$class": "[email protected]",
        "name": "web",
        "version": "1.0.0",
        "commands": [
            {
                "$class": "[email protected]",
                "type": "UPSERT",
                "target": {
                    "$class": "[email protected]",
                    "namespace": "[email protected]"
                },
                "decorator": {
                    "$class": "[email protected]",
                    "name": "Form",
                    "arguments": [
                        {
                            "$class": "[email protected]",
                            "value": "inputType"
                        },
                        {
                            "$class": "[email protected]",
                            "value": "text"
                        }
                    ]
                }
            }
        ]
    }

We get the following output

namespace [email protected]

@Form("inputType","text")
scalar SSN extends String

@Editable
@Form("inputType","text")
concept Person {
  @Custom
  o String firstName
  o String lastName
  o String bio
  o SSN ssn
  o String address1
  o String address2
  o String city
  o String country
  o Integer zip
  o Dictionary dictionary
}

@Form("inputType","text")
map Dictionary {
  o String
  o String
}

@Form("inputType","text")
map Rolodex {
  o String
  o String
}

Expected Behavior

We should get the following output:

@Form("inputType","text")
namespace [email protected]

scalar SSN extends String

@Editable
concept Person {
  @Custom
  o String firstName
  o String lastName
  o String bio
  o SSN ssn
  o String address1
  o String address2
  o String city
  o String country
  o Integer zip
  o Dictionary dictionary
}

map Dictionary {
  o String
  o String
}

map Rolodex {
  o String
  o String
}

Possible Solution

  1. Breaking change - fix the logic which targets namespace. This would be a breaking change for the users who use namespace targeted dcs to apply decorators to all the decorators in that namespace.
  2. Introduce a new property applyToAllDeclarationsInNamespace or something better, which defaults to true. Can be set to false if we don't want to apply to declaration and only at the top of the namespace. Validation could be tricky and would be confusing for users to pickup.
  3. New target option target: "something-new", which would target only the namespace,

I'm more inclined towards the first options if we are able check if the impact is minimal or null. Or else the 3rd option, but I feel we will have to come up with some property value for the target that signal that this is for applying decorator at the top of a namespace, which I feel namespace itself conveys the best.

Steps to Reproduce

https://replit.com/@sanketshevkar/AccordProjectConcerto-Decorator-Command-Set

Context (Environment)

Desktop

  • OS: [e.g. macOS]
  • Browser: [e.g. Chrome, Safari]
  • Version: [e.g. 0.22.15]

Detailed Description

Possible Implementation

@dselman
Copy link
Contributor

dselman commented Jul 8, 2024

I lean towards option one as well. Perhaps controlled by a new flag passed to DecoratorManager?

@sanketshevkar
Copy link
Member Author

I lean towards option one as well. Perhaps controlled by a new flag passed to DecoratorManager?

Something like ENABLE_NAMESPACE_TARGET_FIX?

@sanketshevkar sanketshevkar self-assigned this Jul 9, 2024
@sanketshevkar
Copy link
Member Author

I'll also like to propose the use of [process.emitWarning()](https://nodejs.org/api/process.html#processemitwarningwarning-type-code-ctor) we can emit [DeprecationWarning](https://nodejs.org/api/process.html#nodejs-warning-names) that can caught, handled or suppressed as needed by the consumer of our APIs.
Even NodeJS uses this to issue their deprecation warnings and we can create our own custom warnings. I was not able find if console.warn supports similar features. It just outputs the warning to stderr.

@sanketshevkar
Copy link
Member Author

sanketshevkar commented Jul 9, 2024

emitWarning(`Functionality for namespace targeted Decorator Command Sets has beed changed.`, {
    type: 'DeprecationWarning',
    code: 'concerto-dep:001',
    detail: 'See https://concerto.accordproject.org/dcs-target-namespace'
});

@sanketshevkar
Copy link
Member Author

I had an assumption that we could target decorators at Concept, ConceptDeclaration etc types, as well from the metamodel model. But that isn't the case, should we that add that functionality as well? That would be helpful for consumers who would be using the deprecated logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants