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

Ensure model name is used in namespace for model as suggested above #11732

Closed
4 tasks
Tracked by #11543
nkylstad opened this issue Nov 30, 2023 · 8 comments · Fixed by #12402 or #12504
Closed
4 tasks
Tracked by #11543

Ensure model name is used in namespace for model as suggested above #11732

nkylstad opened this issue Nov 30, 2023 · 8 comments · Fixed by #12402 or #12504
Labels
area/data-modeling Area: Related to data models - e.g. create, edit, use data models. backend breaking-change Status: Will break backwards compatibility or delete data.

Comments

@nkylstad
Copy link
Member

nkylstad commented Nov 30, 2023

Description

⚠️ This is a breaking change! ⚠
We need to ensure that we implement this change without affecting our users too much.

See parent task for more details.

Alternatives

  • Only apply this to apps with v8 or later
    • This will not work very well once people start upgrading their apps 🤔
  • Only apply this to apps with multiple data models
    • Need to consider how to warn users who will get changes in their models/code because of this.
  • - Global usings
@nkylstad nkylstad added the breaking-change Status: Will break backwards compatibility or delete data. label Nov 30, 2023
@nkylstad nkylstad added the area/data-modeling Area: Related to data models - e.g. create, edit, use data models. label Dec 19, 2023
@nkylstad nkylstad moved this to 📈 Todo in Team Studio Dec 19, 2023
@nkylstad
Copy link
Member Author

After discussing with team, we are attempting to solve this with global usings to avoid a breaking change here.

@nkylstad nkylstad moved this from 📈 Todo to ⚠️ Blocked in Team Studio Feb 12, 2024
@nkylstad nkylstad moved this from ⚠️ Blocked to 📈 Todo in Team Studio Feb 12, 2024
@mirkoSekulic mirkoSekulic moved this from 📈 Todo to 👷 In Progress in Team Studio Feb 14, 2024
@mirkoSekulic mirkoSekulic self-assigned this Feb 14, 2024
@mirkoSekulic mirkoSekulic linked a pull request Feb 27, 2024 that will close this issue
4 tasks
@mirkoSekulic mirkoSekulic moved this from 👷 In Progress to 🔎 Review in Team Studio Feb 27, 2024
@github-project-automation github-project-automation bot moved this from 🔎 Review to 🧪 Test in Team Studio Feb 28, 2024
@mirkoSekulic
Copy link
Collaborator

@nkylstad It's implemented in the way that the new namespace is used for new models, and the "old" namespace is maintained if the datamodel is already added in the app to avoid introducing the braking change.

This should be well-tested before publishing to production.

@nkylstad
Copy link
Member Author

@mirkoSekulic So model namespace (for new models) is now postfixed with the model name?
@tjololo @RonnyB71 is this an upgrade that should be included as part of the v8 upgrade tool for existing apps?

@mirkoSekulic
Copy link
Collaborator

@nkylstad yes, new namespace is postfixed with model name.
I'm not sure if it's necessary to include this in v8 upgrade tool. The old apps will work just fine with the models as they are.

@nkylstad
Copy link
Member Author

True 🤔 And if they add a new model and want to be able to access multiple models, that should still work as well, as long as they include the namespace for the new model, right?

@mirkoSekulic
Copy link
Collaborator

@nkylstad correct.

@nkylstad
Copy link
Member Author

nkylstad commented Feb 28, 2024

Tested the following in dev:

  • Removed all models for an existing app, and added a new model:
    • New model had model name postfixed in namespace as expected.
  • In an existing app, edited a model:
    • Model was updated, and namespace remained the same, as expected.
  • In an app with existing models, added a new model:
    • No changes in old model, new model has model name postfixed in namespace, as expected.
  • Created a new app and added two data models:
    • One was added as a new model, and got namespace with postfix of model name as expected
    • One was uploaded, and got namespace with postfix of model name/base class as expected

One thing I did notice was that uploading a model that has the same base class name as an existing model resulted in both models getting the same namespace, which is exactly what we are trying to avoid.
It is not possible to add 2 new models with the same name, but uploading an XSD where the root element (not the file name) is the same as an existing model name/base class is possible and will probably cause issues.

Tried:

  • creating a model with name modell and "Generate models".
  • then uploading this xsd with root node name modell.

Both resulted in the same namespace and the same root class, which will probably cause an issue when running the app 🤔

@nkylstad nkylstad moved this from 🧪 Test to 👀 Test feedback in Team Studio Feb 28, 2024
@framitdavid framitdavid moved this from 👀 Test feedback to 👷 In Progress in Team Studio Mar 8, 2024
@nkylstad nkylstad reopened this Mar 11, 2024
@mirkoSekulic mirkoSekulic moved this from 👷 In Progress to 🔎 Review in Team Studio Mar 14, 2024
@mirkoSekulic mirkoSekulic linked a pull request Mar 14, 2024 that will close this issue
4 tasks
@standeren standeren assigned standeren and unassigned mirkoSekulic Mar 15, 2024
@github-project-automation github-project-automation bot moved this from 🔎 Review to 🧪 Test in Team Studio Mar 18, 2024
@standeren standeren removed their assignment Mar 18, 2024
@nkylstad
Copy link
Member Author

nkylstad commented Apr 8, 2024

Re-tested the same case as above.
Upload fails with status code 422 and error code "ModelWithTheSameTypeNameExists". So this now works as expected.

In our frontend, we only show the generic error message:
Image

This is because the XSDUpload component calls axios directly, instead of using react query. So it does not get the automatic error handling with toasts. We already have the error message for the "ModelWithTheSameTypeNameExists" error code.
We should, at a minimum, update XSDUpload to check the error response and use the relevant error code.

Or, preferably we should use react query in this component. Will create a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/data-modeling Area: Related to data models - e.g. create, edit, use data models. backend breaking-change Status: Will break backwards compatibility or delete data.
Projects
Archived in project
3 participants