Skip to content

Issue 52339: Try to use shorter URIs for domains when possible #6805

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.

Sign up for GitHub

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

Merged
labkey-susanh merged 7 commits into from
Jul 1, 2025
Merged

Issue 52339: Try to use shorter URIs for domains when possible #6805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7859c68
Issue 52339: Change list domain URI construction to not use encoded u…
labkey-susanh Jun 26, 2025
bb3bfb8
Check for null domain kind
labkey-susanh Jun 27, 2025
808479d
Merge remote-tracking branch 'origin/develop' into fb_domainLsids
labkey-susanh Jun 27, 2025
077c4ea
Add comment
labkey-susanh Jun 27, 2025
d85f8ca
Merge remote-tracking branch 'origin/develop' into fb_domainLsids
labkey-susanh Jun 30, 2025
3ffd027
Remove unneeded parameters
labkey-susanh Jun 30, 2025
26667c6
Merge remote-tracking branch 'origin/develop' into fb_domainLsids
labkey-susanh Jun 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions api/src/org/labkey/api/exp/LsidManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import org.labkey.api.assay.AssayUrls;
import org.labkey.api.data.Container;
import org.labkey.api.data.ContainerManager;
import org.labkey.api.data.DbSequence;
import org.labkey.api.data.DbSequenceManager;
import org.labkey.api.data.SimpleFilter;
import org.labkey.api.data.Sort;
import org.labkey.api.data.TableSelector;
Expand Down Expand Up @@ -72,6 +74,20 @@ public static LsidManager get()
return INSTANCE;
}

public static DbSequence getLsidPrefixDbSeq(String lsidPrefix, int batchSize)
{
return getLsidPrefixDbSeq(ContainerManager.getRoot(), lsidPrefix, batchSize);
}

public static DbSequence getLsidPrefixDbSeq(Container container, String lsidPrefix, int batchSize)
Copy link
Contributor

@labkey-jeckels labkey-jeckels Jun 27, 2025

Choose a reason for hiding this comment

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

Presumably this is safe because container RowIds are included in many LSIDs, but since containers can be moved, would it make sense to have a single sequence used server-wide?

Copy link
Contributor Author

@labkey-susanh labkey-susanh Jun 30, 2025

Choose a reason for hiding this comment

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

I can certainly change the usage for Lists so it uses the root container. If this method is updated now, we'd need to change the usage from the Experiment module so it can skip over LSIDs already in use from the project-scoped LSIDs.

@XingY Was there a reason not to make this server-wide when you were adding these sequences in ExperimentServiceImpl?

Copy link
Contributor

@XingY XingY Jun 30, 2025

Choose a reason for hiding this comment

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

Historically we had always included folder / container as part of lsid. The change I introduced was switching from data type name to use a db sequence when generating lsid, in order to support datatype/data renaming. LSIDs are not updated during a folder move, and shouldn't cause issues/conflict.

Copy link
Contributor Author

@labkey-susanh labkey-susanh Jun 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It isn't a move that would cause the conflict but the restarting of the sequence if we switch it to use a different container where the conflict may occur. (Previous project's sequence value of 1 vs. new project's sequence value of 1.) But, yes, when there is a folder id in the LSID, there won't be a problem here.

Copy link
Contributor Author

@labkey-susanh labkey-susanh Jul 1, 2025

Choose a reason for hiding this comment

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

My future self may not thank me for this, but I'm opting for leaving the Experiment usages as is and adding a method that provides for an easy way to use a root container sequence, which we'll use for lists.

{
Container projectContainer = container; // use DBSeq at project level to avoid duplicate lsid for types in child folder
if (!container.isProject() && container.getProject() != null)
projectContainer = container.getProject();

return DbSequenceManager.getPreallocatingSequence(projectContainer, ExperimentService.LSID_COUNTER_DB_SEQUENCE_PREFIX + lsidPrefix, 0, batchSize);
}

public interface LsidHandler<I extends Identifiable>
{
I getObject(Lsid lsid);
Expand Down
2 changes: 1 addition & 1 deletion api/src/org/labkey/api/exp/property/Domain.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public interface Domain extends IPropertyType

Object get_Ts();
Container getContainer();
DomainKind<?> getDomainKind();
@Nullable DomainKind<?> getDomainKind();
String getName();
String getTitle();
String getDescription();
Expand Down
3 changes: 2 additions & 1 deletion experiment/src/org/labkey/experiment/api/ExpSampleTypeImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.labkey.api.exp.ChangePropertyDescriptorException;
import org.labkey.api.exp.ExperimentException;
import org.labkey.api.exp.Lsid;
import org.labkey.api.exp.LsidManager;
import org.labkey.api.exp.PropertyColumn;
import org.labkey.api.exp.api.ExpData;
import org.labkey.api.exp.api.ExpMaterial;
Expand Down Expand Up @@ -964,7 +965,7 @@ public Lsid.LsidBuilder generateNextDBSeqLSID()

public DbSequence getSampleLsidDbSeq(int batchSize, Container container)
{
return ExperimentServiceImpl.getLsidPrefixDbSeq(container, "SampleType-" + getRowId(), batchSize);
return LsidManager.getLsidPrefixDbSeq(container, "SampleType-" + getRowId(), batchSize);
}

@Override
Expand Down
12 changes: 1 addition & 11 deletions experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@
import org.labkey.api.data.DbSchema;
import org.labkey.api.data.DbSchemaType;
import org.labkey.api.data.DbScope;
import org.labkey.api.data.DbSequence;
import org.labkey.api.data.DbSequenceManager;
import org.labkey.api.data.JdbcType;
import org.labkey.api.data.NameGenerator;
Expand Down Expand Up @@ -1633,20 +1632,11 @@ public static String getNamespacePrefix(Class<? extends ExpObject> clazz)

private Pair<String, String> generateLSIDWithDBSeq(Container container, String lsidPrefix)
{
String dbSeqStr = String.valueOf(getLsidPrefixDbSeq(container, lsidPrefix, 1).next());
String dbSeqStr = String.valueOf(LsidManager.getLsidPrefixDbSeq(container, lsidPrefix, 1).next());
String lsid = generateLSID(container, lsidPrefix, dbSeqStr);
return new Pair<>(lsid, dbSeqStr);
}

public static DbSequence getLsidPrefixDbSeq(Container container, String lsidPrefix, int batchSize)
{
Container projectContainer = container; // use DBSeq at project level to avoid duplicate lsid for types in child folder
if (!container.isProject() && container.getProject() != null)
projectContainer = container.getProject();

return DbSequenceManager.getPreallocatingSequence(projectContainer, LSID_COUNTER_DB_SEQUENCE_PREFIX + lsidPrefix, 0, batchSize);
}

private String generateGuidLSID(Container container, String lsidPrefix)
{
return generateLSID(container, lsidPrefix, GUID.makeGUID());
Expand Down
2 changes: 1 addition & 1 deletion experiment/src/org/labkey/experiment/api/property/DomainImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ public Container getContainer()
return _dd.getContainer();
}

@Override
@Override @Nullable
public DomainKind<?> getDomainKind()
{
return _dd.getDomainKind();
Expand Down
2 changes: 1 addition & 1 deletion experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1808,7 +1808,7 @@ public Set<String> getReservedPropertyNames(Domain domain, User user)
{
d = new DomainImpl(c, uri, "test", true)
{
@Override
@Override @Nullable
public DomainKind<?> getDomainKind()
{
return k;
Expand Down
22 changes: 19 additions & 3 deletions experiment/src/org/labkey/experiment/defaults/DefaultValueServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.labkey.api.exp.OntologyManager;
import org.labkey.api.exp.api.ExperimentService;
import org.labkey.api.exp.property.Domain;
import org.labkey.api.exp.property.DomainKind;
import org.labkey.api.exp.property.DomainProperty;
import org.labkey.api.gwt.client.DefaultValueType;
import org.labkey.api.query.ValidationException;
Expand Down Expand Up @@ -59,13 +60,28 @@ public class DefaultValueServiceImpl implements DefaultValueService
private String getContainerDefaultsLSID(Container container, Domain domain)
{
String suffix = "Folder-" + container.getRowId();
return (new Lsid(DOMAIN_DEFAULT_VALUE_LSID_PREFIX, suffix, domain.getName())).toString();
DomainKind<?> kind = domain.getDomainKind();
Copy link
Contributor

@XingY XingY Jun 30, 2025

Choose a reason for hiding this comment

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

Would this cause backward compatibility issue? If a default value is set prior to this change, would it be lost since the default lsid is different now?

Copy link
Contributor Author

@labkey-susanh labkey-susanh Jun 30, 2025

Choose a reason for hiding this comment

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

I think this will be ok because when the domain is user-defined, the objectId from the domain LSID is the same as the encoded name. I tested it locally, but it would be good to double check by creating a list while on develop that has some default values defined for properties then verifying the default values still show up when using the list on this branch.

if (kind != null && kind.isUserCreatedType())
{
Lsid domainLsid = new Lsid(domain.getTypeURI());
return (new Lsid(DOMAIN_DEFAULT_VALUE_LSID_PREFIX, suffix, Lsid.decodePart(domainLsid.getObjectId()))).toString();
}
else // for internal domains (such as audit domains), the domain name and objectId are often not the same.
return (new Lsid(DOMAIN_DEFAULT_VALUE_LSID_PREFIX, suffix, domain.getName())).toString();

}

private String getUserDefaultsParentLSID(Container container, User user, Domain domain)
{
String suffix = "Folder-" + container.getRowId() + ".User-" + user.getUserId();
return (new Lsid(USER_DEFAULT_VALUE_DOMAIN_PARENT, suffix, domain.getName())).toString();
DomainKind<?> kind = domain.getDomainKind();
if (kind != null && kind.isUserCreatedType())
{
Lsid domainLsid = new Lsid(domain.getTypeURI());
return (new Lsid(USER_DEFAULT_VALUE_DOMAIN_PARENT, suffix, Lsid.decodePart(domainLsid.getObjectId()))).toString();
}
else // for internal domains (such as audit domains), the domain name and objectId are often not the same.
return (new Lsid(USER_DEFAULT_VALUE_DOMAIN_PARENT, suffix, domain.getName())).toString();
}

private static final String WILD_CARD_PLACEHOLDER = "WILDCARD";
Expand Down Expand Up @@ -390,4 +406,4 @@ public boolean hasDefaultValues(Container container, Domain domain, User user, S
}
return false;
}
}
}
2 changes: 1 addition & 1 deletion list/src/org/labkey/list/model/ListDefinitionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public ListDefinitionImpl(Container container, String name, KeyType keyType, @Nu
builder.setKeyType(keyType.toString());
builder.setCategory(category);
_def = builder;
Lsid lsid = ListDomainKind.generateDomainURI(name, container, keyType, category);
Lsid lsid = ListDomainKind.generateDomainURI(container, keyType, category);
_domain = PropertyService.get().createDomain(container, lsid.toString(), name, templateInfo);
}

Expand Down
22 changes: 11 additions & 11 deletions list/src/org/labkey/list/model/ListDomainKind.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.labkey.api.data.ColumnInfo;
import org.labkey.api.data.Container;
import org.labkey.api.data.ContainerFilter;
import org.labkey.api.data.ContainerManager;
import org.labkey.api.data.DbScope;
import org.labkey.api.data.JdbcType;
import org.labkey.api.data.PropertyStorageSpec;
Expand All @@ -39,6 +40,7 @@
import org.labkey.api.di.DataIntegrationService;
import org.labkey.api.exp.DomainNotFoundException;
import org.labkey.api.exp.Lsid;
import org.labkey.api.exp.LsidManager;
import org.labkey.api.exp.OntologyManager;
import org.labkey.api.exp.PropertyDescriptor;
import org.labkey.api.exp.PropertyType;
Expand Down Expand Up @@ -268,20 +270,18 @@ public Set<PropertyStorageSpec.Index> getPropertyIndices(Domain domain)
return Collections.emptySet(); // TODO: Allow this to return the Key Column
}

public static Lsid generateDomainURI(String name, Container c, KeyType keyType, @Nullable ListDefinition.Category category)
public static Lsid generateDomainURI(Container c, KeyType keyType, @Nullable ListDefinition.Category category)
{
String type = getType(keyType, category);
Copy link
Contributor

@XingY XingY Jun 30, 2025

Choose a reason for hiding this comment

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

Not related to this PR, but surprised to see category is hard wired to picklist.
if (category != null) type = PicklistDomainKind.NAMESPACE_PREFIX;

StringBuilder typeURI = getBaseURI(name, type, c);

// Issue 13131: uniqueify the lsid for situations where a preexisting list was renamed
int i = 1;
String sTypeURI = typeURI.toString();
String uniqueURI = sTypeURI;
while (OntologyManager.getDomainDescriptor(uniqueURI, c) != null)
Lsid lsid;
// assure LSID does not collide with previous lsids that may have had number names
do
{
uniqueURI = sTypeURI + '-' + (i++);
}
return new Lsid(uniqueURI);
String dbSeqStr = String.valueOf(LsidManager.getLsidPrefixDbSeq(type, 1).next());
lsid = new Lsid(type, "Folder-" + c.getRowId(), dbSeqStr);
} while (OntologyManager.getDomainDescriptor(lsid.toString(), c) != null);

return lsid;
}

public static Lsid createPropertyURI(String listName, String columnName, Container c, ListDefinition.KeyType keyType)
Expand Down