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

complete limits in branch creation #572

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

Mathieu-Deharbe
Copy link
Contributor

@Mathieu-Deharbe Mathieu-Deharbe commented Dec 10, 2024

needs network-modification lib update in order to compile :
gridsuite/network-modification#9

Temp files that will be removed before merging :
pom.xml
src/main/resources/db/changelog/changesets/différence avec main.xml

I keep these here temporarily for facilitating the dev


ATTENTION migration liquidbase included.

-> may be a good idea to remove the delete of the obsolete data column in order to keep the data just in case of problem during the migration

@Mathieu-Deharbe Mathieu-Deharbe changed the title draft complete limits in branch creation (not tested) complete limits in branch creation Dec 12, 2024
Signed-off-by: Mathieu DEHARBE <[email protected]>
), nullable = true)
private CurrentLimitsEntity currentLimits2;
@OneToMany(cascade = CascadeType.ALL, orphanRemoval = true)
@OrderColumn
Copy link
Contributor

Choose a reason for hiding this comment

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

@OrderColumn(name = "pos_currentLimits1")

Copy link
Contributor Author

@Mathieu-Deharbe Mathieu-Deharbe Dec 18, 2024

Choose a reason for hiding this comment

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

Ok but renamed @OrderColumn(name = "pos_operationalLimitsGroups1") given that I changed the structure.

private List<CurrentLimitsEntity> currentLimits1;

@OneToMany(cascade = CascadeType.ALL, orphanRemoval = true)
@OrderColumn
Copy link
Contributor

Choose a reason for hiding this comment

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

@OrderColumn(name = "pos_currentLimits2")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok but renamed @OrderColumn(name = "pos_operationalLimitsGroups2") given that I changed the structure.

name = "current_limits_id2_fk"
), nullable = true)
private CurrentLimitsEntity currentLimits2;
@OneToMany(cascade = CascadeType.ALL, orphanRemoval = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

you removed the @joincolumn ?

Copy link
Contributor Author

@Mathieu-Deharbe Mathieu-Deharbe Dec 18, 2024

Choose a reason for hiding this comment

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

Yes. Now that this is a list instead of a single object, a join column can't reference the whole list of the entry.

pom.xml Outdated
@@ -129,7 +129,7 @@
<dependency>
<groupId>org.gridsuite</groupId>
<artifactId>gridsuite-network-modification</artifactId>
<version>${network-modification.version}</version>
<version>0.3.0-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

to be removed

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 but I keep it here to simplify my test for now.

I precised in the PR description that 2 changes have to be removed before the merging. This is one of them.

connectionDirection1 = branchCreationInfos.getConnectionDirection1();
connectionName1 = branchCreationInfos.getConnectionName1();
connectionDirection2 = branchCreationInfos.getConnectionDirection2();
connectionName2 = branchCreationInfos.getConnectionName2();
selectedOperationalLimitsGroupId1 = branchCreationInfos.getSelectedOperationalLimitsGroupId1();
Copy link
Contributor

Choose a reason for hiding this comment

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

getActiveOperationalLimits ?

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 could have been a good name but I use the iidm terminology. Cf LineAttributes.java in powsybl network store.

@@ -25,6 +25,8 @@
@Embeddable
public class CurrentTemporaryLimitCreationEmbeddable {

// TODO : pas besoin d'id ici ??
Copy link
Contributor

@basseche basseche Dec 18, 2024

Choose a reason for hiding this comment

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

forgotten todo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. It already worked without it. I was too suspicious because of a bug.

pom.xml Outdated
@@ -129,7 +129,7 @@
<dependency>
<groupId>org.gridsuite</groupId>
<artifactId>gridsuite-network-modification</artifactId>
<version>${network-modification.version}</version>
<version>0.4.0-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think you should commit this !

Copy link
Contributor

Choose a reason for hiding this comment

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

rollback 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 but I keep it here to simplify my test for now.

I precised in the PR description that 2 changes have to be removed before the merging. This is one of them.

</changeSet>
<changeSet author="deharbemat (generated)" id="1734603384951-26">
<createTable tableName="two_windings_transformer_creation_operational_limits_groups1">
<column name="two_windings_transformer_creation_entity_id" type="UUID">
Copy link
Contributor

Choose a reason for hiding this comment

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

can we reduce column name ? like for example creation_entity_id ?

Copy link
Contributor

Choose a reason for hiding this comment

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

To fix the name column, it should be done properly in the entity class and not in the changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. It is automatically generated by this :

@OneToMany(cascade = CascadeType.ALL, orphanRemoval = true)
    @OrderColumn(name = "pos_operationalLimitsGroups1")
    private List<OperationalLimitsGroupEntity> operationalLimitsGroups1;

The problem is that this is part of BranchCreationEntity inherited by transfos and lines. I don't really know how to rename it properly.

), nullable = true)
private CurrentLimitsEntity currentLimits2;
@OneToMany(cascade = CascadeType.ALL, orphanRemoval = true)
@OrderColumn(name = "pos_operationalLimitsGroups1")
Copy link
Contributor

Choose a reason for hiding this comment

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

operationalLimitsGroup1 (whithout s) to be consistent with the code on the library side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here operationalLimitsGroups1 is a list because there are several groups. So I think the 's' makes sense.

connectionPosition1 = branchCreationInfos.getConnectionPosition1();
connectionPosition2 = branchCreationInfos.getConnectionPosition2();
connected1 = branchCreationInfos.isConnected1();
connected2 = branchCreationInfos.isConnected2();
}

/**
* the point of this function is to never dereference operationalLimitsGroups if it already exists,
Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose of this function is to avoid dereferencing operationalLimitsGroups if it already exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK done.


/**
* the point of this function is to never dereference operationalLimitsGroups if it already exists,
* in order for Hibernate not to lose this reference when doing some cascade cleaning or whatever
Copy link
Contributor

Choose a reason for hiding this comment

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

in order to prevent Hibernate from losing the reference during cascade cleaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK done.

referencedColumnName = "id",
foreignKey = @ForeignKey(
name = "current_limits_id_fk"
), nullable = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the nullable parameter (by default is true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK done.

@Column(name = "uuid")
private UUID uuid;

@Schema(description = "Operational limit group id")
Copy link
Contributor

Choose a reason for hiding this comment

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

@Schema in the entity ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected into :

    @Column(name = "id")
    private String id;

updatedLimitsGroups = new ArrayList<>();
} else {
updatedLimitsGroups.clear();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

List<OperationalLimitsGroupEntity> updatedLimitsGroups = (operationalLimitsGroups == null)
                ? new ArrayList<>()
                : new ArrayList<>(operationalLimitsGroups);
        }

maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try but I think it won't work because new ArrayList<>(operationalLimitsGroups); may lose the reference to operationalLimitsGroups by creating a new Array.

@NoArgsConstructor
@AllArgsConstructor
@Entity
@Table(name = "operationalLimitsGroup")
Copy link
Contributor

@ghazwarhili ghazwarhili Jan 6, 2025

Choose a reason for hiding this comment

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

@table(operational_limits_group)
@PrimaryKeyJoinColumn(foreignKey = @foreignkey(name = "operational_limits_group_id_fk_constraint"))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cna't find this. Is it outdated or can you find it ?

* @author Mathieu Deharbe <mathieu.deharbe_externe at rte-france.com>
*/
@Getter
@Setter
Copy link
Contributor

Choose a reason for hiding this comment

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

you need the @Setter 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.

Apparently not. Removed.

Mathieu-Deharbe and others added 4 commits January 6, 2025 18:26
@ghazwarhili
Copy link
Contributor

[ERROR] Tests run: 843, Failures: 2, Errors: 0, Skipped: 0

Signed-off-by: Mathieu DEHARBE <[email protected]>
@Mathieu-Deharbe
Copy link
Contributor Author

[ERROR] Tests run: 843, Failures: 2, Errors: 0, Skipped: 0

Indeed sorry, I corrected it.

This is caused by the change from selectedOperationalLimitsGroupId2 to selectedOperationalLimitsGroup2 that made me change stuff everywhere. And I may have to go back because it has taken me away from the IIDM standard (which uses selectedOperationalLimitsGroupId2).

Mathieu-Deharbe and others added 4 commits January 15, 2025 11:13
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.

3 participants