-
Notifications
You must be signed in to change notification settings - Fork 7
feat(aasrepository): restore new implementation after sync with main #90
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
base: main
Are you sure you want to change the base?
feat(aasrepository): restore new implementation after sync with main #90
Conversation
Martin187187
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You’ve implemented the new endpoints with a minimal version. This is a good starting point. Keep in mind that AssetAdministrationShell contains many more fields, so I assume this was meant as a prototype. I’ve added a few comments for future iterations:
- Please use goqu as our query builder. Make sure to review existing patterns so the queries remain consistent with the rest of the codebase.
- Make use of the global schema file. Many tables and subqueries can be reused, and this will help avoid duplication and ensure consistent structure.
- Use our standard error response format. I included an example in the comments, all new endpoints should follow this structure.
Let me know if you need any guidance integrating these points.
internal/aasrepository/api/api_asset_administration_shell_repository_api_service.go
Outdated
Show resolved
Hide resolved
internal/aasrepository/api/api_asset_administration_shell_repository_api_service.go
Outdated
Show resolved
Hide resolved
internal/aasrepository/api/api_asset_administration_shell_repository_api_service.go
Outdated
Show resolved
Hide resolved
… Administration, AssetInformation, DerivedFrom
Martin187187
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve reviewed your changes and see two critical points:
-
Scalability of the GET request: The current implementation does not scale well with the number of AAS entries in the database. Ideally, the number of queries should remain constant, regardless of how much data is stored.
-
Code reuse and batching: A significant amount of existing code could be reused (e.g. LangStringNameTypes, LangStringTextTypes, SpecificAssetIds). These utilities are already highly optimized because they operate in batches. Reusing them would both improve performance and increase overall code quality and consistency.
The first point is particularly important from a scalability perspective. The second point is more about maintainability and quality, but it could still have a positive performance impact.
Feel free to share your thoughts—happy to discuss trade-offs or alternative approaches.
|
|
||
|
|
||
| CREATE TABLE IF NOT EXISTS aas_extension ( | ||
| aas_id VARCHAR(2048) NOT NULL REFERENCES aas(id) ON DELETE CASCADE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the primary key to reference to aas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Martin187187 Could you help me understand this comment? 'id' is the PK for the table aas
CREATE TABLE IF NOT EXISTS aas ( id VARCHAR(2048) PRIMARY KEY, id_short VARCHAR(2048), category VARCHAR(2048), model_type VARCHAR(128) NOT NULL, administration_id BIGINT REFERENCES administrative_information(id), asset_information_id BIGINT REFERENCES asset_information(id), derived_from_reference_id BIGINT REFERENCES reference(id) );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my explanation is extremly bad here. What i meant:
- Create a new primary key in aas that is an integer.
- make id unique in aas table. use the new primary key for references as it is more efficient to use integer in relationships (less byte)
This doesnt change any logic but would be best practice for designing schemas.
| return ai, nil | ||
| } | ||
|
|
||
| func (p *PostgreSQLAASDatabase) insertReference(ref *model.Reference) (int64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| // insertDisplayName inserts the DisplayName entries for the AAS. | ||
| func (p *PostgreSQLAASDatabase) insertDisplayName(aas model.AssetAdministrationShell) error { | ||
| if len(aas.DisplayName) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } | ||
|
|
||
| // insertDescription inserts the Description entries for the AAS. | ||
| func (p *PostgreSQLAASDatabase) insertDescription(aas model.AssetAdministrationShell) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // 3) Specific Asset IDs | ||
| for _, sid := range aas.AssetInformation.SpecificAssetIds { | ||
| query, _, err = dialect. | ||
| Insert("aas_specific_asset_id"). | ||
| Rows(goqu.Record{ | ||
| "asset_information_id": assetInfoID, | ||
| "name": sid.Name, | ||
| "value": sid.Value, | ||
| "semantic_id": sid.SemanticID, | ||
| "external_subject_id": sid.ExternalSubjectID, | ||
| }). | ||
| ToSQL() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if _, err = p.DB.Exec(query); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could use existing code if you reuse the specific asset table: https://github.com/Prajwala-Adiga/basyx-go-components/blob/aasrepository-v2/internal/common/descriptors/WriteSpecificAssetId.go
| for _, d := range aas.Description { | ||
| query, _, err = dialect. | ||
| Insert("lang_string_text_type"). | ||
| Rows(goqu.Record{ | ||
| "lang_string_text_type_reference_id": descRefID, | ||
| "language": d.Language, | ||
| "text": d.Text, | ||
| }). | ||
| ToSQL() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if _, err = p.DB.Exec(query); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could reuse code that handles that in a batched requests: https://github.com/Prajwala-Adiga/basyx-go-components/blob/aasrepository-v2/internal/common/descriptors/ReadLanguages.go
FriedJannik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some things i caught - i think Martin reviewed the rest :)
basyxschema.sql
Outdated
| CREATE TYPE modelling_kind AS ENUM ('Instance', 'Template'); | ||
| END IF; | ||
| END $$; | ||
| END; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of these changes - are those necessarry? If o, would you mind reverting them so that the diff is not that large ? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were the changes that were giving me compilation errors. So, I was forced to make these changes. But to give the benefit of doubt, I can revert them and test them again for completeness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
basyxschema.sql
Outdated
|
|
||
| CREATE TABLE IF NOT EXISTS asset_information ( | ||
| id BIGSERIAL PRIMARY KEY, | ||
| asset_kind VARCHAR(64), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the ENUM here (cf ll.136-141)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
cmd/aasrepositoryservice/Dockerfile
Outdated
|
|
||
|
|
||
| # ---------- Runtime stage ---------- | ||
| FROM alpine:3.19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use alpine 3.22 here aswell
| FROM alpine:3.19 | |
| FROM alpine:3.22 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file needed?
|
I just noticed that the current specficAssetId table has descriptorId as a column. Therefore this table cannot be reused. |
… to avoid compile errors
Description of Changes
This pull request introduces the initial implementation of the Asset Administration Shell (AAS) Repository service in the BaSyx Go components.
#Key Features Added
AAS Repository Service
PostgreSQL Persistence Layer
Database Schema Integration
AAS Data Model Support
Testing & Quality Assurance
Configuration & Tooling
Related Issue #66
Notes for Reviewers
AAS columns (Extensions, EmbeddedDataSpecifications, Submodel linking) are not implemented. They are planned for follow-up PRs once the foundation is reviewed and merged.