This class allows us to generalize supporting the AssignIdentifierJob that batch generates ids for rows where the id is null. I made it a separate trait from IdentifierService because not every implementation needs to support it.

this is one of the examples of how we handle checking multiple tables for uniqueness when generating an id.

Thanks. I was going to ask about how we handle complex cases like multiple tables.

1. Could we change the argument to stay consistent with the abstract class method. Or do you feel this is an important detail for someone trying to figure out what's going on?

    countDuplicates(String identifier)
   

2. We should add a comment here to explain what's happening and include a FIXME to remind us that we need to fix this logic once we unmingle the identifiers.

We essentially want to support the following

• a requisition can be associated with multiple shipments
• shipments should always get their own shipment number assigned
• shipment number != requisition number

for 1 and 2, yes can do. Thanks for the context on the why. Maybe we can add a dev task in the next release to refactor this properly, or at least a discovery to determine the work required. The nice thing about the ids being the same is that it's simpler to cross-reference them, but yes if we're refactoring to not need them, we could remove the check here on other tables.

Another option would be to not check for duplicates in other tables at all, and instead of simply assigning shipmentNumber=requestNumber, we do something like:

shipmentNumber = shipmentIdentifierService.generate(IdentifierGeneratorParams.builder()
                .formatOverride("\${custom.requestNumber}-\${random}")
                .customKeys(['requestNumber': requisition.requestNumber])
                .build())

So given requestNumber ABC123, shipmentNumber would be ABC123-123456.

A majority of this code is just copied out from IdentifierService and some other places. I couldn't get around to refactoring this as a part of this change because it has such a complex configuration.

See the TODO here for how we could refactor in the future.

Yeah this one is though, perhaps we could get more tests for it? This is not a change request for now, but might be an entry point to bigger refactor of that part

A majority of this code is just copied out from IdentifierService and OrderService. I couldn't get around to refactoring this as a part of this change.

See the TODO here for how we could refactor in the future.

I haven't read the TODOs but we can discuss if/when we get another chance to refactor. Or discuss now and add an idea garden ticket to briefly discuss the options.

A majority of this code is just copied out from IdentifierService and some other places. I couldn't get around to refactoring this as a part of this change because it has such a complex configuration.

All I really did here was add comments to clarify the code.

See the TODO here for how we could refactor in the future.

It would be a really good idea to take some time now to think about how all of the different generate() methods work, document their requirements / behaviors, and come up with a design / unifying interface that will allow us to refactor these one-by-one in the future.

• PO identifier requires a sequential identifier that is being stored on the organization
• Product identifiers use a random number identifier unless there's a different format provided by the product type (i don't think we don't currently support sequential number identifiers here, but we should)
• Shipment identifiers are assigned (should be deprecated in favor of separate)
• Requisition identifiers are random number identifiers
• yada yada yada

The TODO describes my goal here. I want to have a single generate() method that is driven entirely by properties. I'm imagining this being configured like:

openboxes.identifier.organization.format = "\${abbreviation}"
openboxes.identifier.organization.abbreviation.field = "name"
openboxes.identifier.organization.minSize = 2
openboxes.identifier.organization.maxSize = 3

then run with:

Organization org = new Organization(name: "A Good Name")
String id = organizationIdentifierService.generate(org)
id == "AGN"

and you can make it as complex as you want. For example:

openboxes.identifier.organization.format = "\${abbreviation}\${random}"
openboxes.identifier.organization.abbreviation.field = "name"
openboxes.identifier.organization.minSize = 2
openboxes.identifier.organization.maxSize = 3
openboxes.identifier.organization.random.template = "-NNNN"
openboxes.identifier.organization.random.condition = RandomCondition.ON_DUPLICATE

which will output "AGN-1234" if "AGN" already exists.

This was an attempt to unify our approach to fetching config.

It's based on best practices defined here: https://grails.org/blog/2016-08-31.html

It also makes mocking config in tests a lot easier.

There's a ticket or two from OBGM to discover best practices on how to properly retrieve config properties in the application (and to avoid the legacy Holder pattern).

I was planning to migrate to using the Value annotation in as many places as possible. But having multiple ways to retrieve the configuration properties (particularly if we cannot inject them via Value in some places) makes sense. If we later determine that the Value annotation gets us all the way there, then we can remove this service.

Note to self: test if we still need this!

Did you check that one?

No unfortunately... I think I'm going to leave it as is for now, but I do think it's worth investigating in the future if this is actually a good idea. I'm thinking it'd be better to split them all into their own sessions/transactions.

Note that I added "default" format and random fallbacks. This simplifies configuration for features that don't need a custom format

Should the properties above also fall under the "default" or "random" namespace since they are the defaults?

Thinking out loud ... I think the first three would make more sense under the "random" namespace since they are only used with the random generator.

openboxes.identifier.random.numeric = "0123456789"
openboxes.identifier.random.alphabetic = "ABCDEFGHJKMNPQRSTUVXYZ"
openboxes.identifier.random.alphanumeric = "0123456789ABCDEFGHJKMNPQRSTUVWXYZ"

The sequence number is separate but that could potentially be under

openboxes.identifier.sequence.format = "00000"

I agree. I didn't change those ones because I wanted to preserve existing properties, but I do think they make sense as random ones specifically. If we're comfortable with breaking backwards compatibility, I can switch them over

Ooh, that's a good point re: maintaining backwards compatibility. I don't actually think we know if anyone has overriden these properties so maybe I was wrong. Maybe we need to keep them for now. We can discuss or I'll leave it up to you to decide.

Ideally, we would provide a mechanism to "deprecate" config properties to allow config property changes like this. We could keep a mapping of old config properties (to their superseding property) for backwards-compatibility.

One brute force way to deal with these changes would be to keep track of the deprecated config properties in a list somewhere and throw an exception during startup if we encounter any of them in the grailsApplication.config. The better approach would be to fallback to these deprecated values (if they exist) but that adds complexity for testing and lifecycle management (when do they die).

In general, we could do something like fetch the new format, then if we find nothing fetch the old format. It's two lookups but only if you're on the old format.

In this case though I agree it's unlikely anyone has modified these properties so I'll just change them.

EDIT: Actually I think I'm going to leave these as is. I don't see much need to provide the ability to override them on a per feature basis (and so don't need a "default" in my opinion), and they're not strictly specific to random (though currently that's all they're used for).

Sounds good.

This is where the bulk of the change is. I generalize the generate flow here and split out any feature specific stuff to feature services. It's also where the retry mechanism is. It's hard to parse in diff view so I suggest reviewing the raw file.

Did you check that one?

Yeah this one is though, perhaps we could get more tests for it? This is not a change request for now, but might be an entry point to bigger refactor of that part

Should the properties above also fall under the "default" or "random" namespace since they are the defaults?

Thinking out loud ... I think the first three would make more sense under the "random" namespace since they are only used with the random generator.

openboxes.identifier.random.numeric = "0123456789"
openboxes.identifier.random.alphabetic = "ABCDEFGHJKMNPQRSTUVXYZ"
openboxes.identifier.random.alphanumeric = "0123456789ABCDEFGHJKMNPQRSTUVWXYZ"

The sequence number is separate but that could potentially be under

openboxes.identifier.sequence.format = "00000"

I assume custom is an argument passed to the identifier generation method but need to understand this better.

I think you figured it out, but just in case, custom is a map that is passed in as a param. In this case:

IdentifierGeneratorParams.builder()
        .templateEntity(order)
        .customKeys(["sequenceNumber": sequenceNumberStr])
        .build())

but for sequenceNumber, this is a halfway solution since I want to eventually integrate that logic with the core id generation flow. Eventually you'll be able to just define openboxes.identifier.purchaseOrder.format = "PO-\${destinationPartyCode}-\${sequenceNumber}" and it'll know to look up the openboxes.identifier.x.sequenceNumber.format property (so no need for the custom key)

We can't get there right now, but it would be great if we could hide the implementation details and expose the identifier generation strategies as an "API" for configuring for each identifier type.

In other words, if a system admin wants to override the transaction identifier, they would first need to know that the transaction uses the random identifier strategy i.e. because that is the default strategy.

openboxes.identifier.default.format = "\${random}"  // By default, simply generate a random id

They would also need to know that the random identifier strategy uses the default template

openboxes.identifier.default.random.template = "NNNLLL"

But there's already a config property for the transaction object ...

openboxes.identifier.transaction.random.template = "AAA-AAA-AAA"

so we just need to override this property.

openboxes.identifier.transaction.random.template = "NNN-NNN-NNN"

Instead, we could just provide an expression language (exposing an API of identifier strategies) to allow the system admin to be explicit about what they want to do.

openboxes.identifier.transaction.format = "#random('AAA-AAA-AAA')"

or it's a sequence number

openboxes.identifier.purchaseOrder.format = "#sequence('00000')"

or even something like this

# annotate the properties that should be included in the hashed value
openboxes.identifier.myObject.format = "#hash()"  
# explicitly define the properties to use in the config
openboxes.identifier.myObject.format = "#hash(['property1', 'property2'])"

The one caveat for the sequence identifier strategy is that we'd need to tell it what column it's targetting, which is a fact known by the caller, but not by the identifier strategy itself.

purchaseOrder.orderNumber = identifierService.generateIdentifier(context)

So we would probably need to build a context to pass to the generateIdentifier method so that it's aware of the identifier config as well as the target of the identifier. But this feels a little gross since it would be nice if the strategy just knew where it look to compare. I guess that could be handled by an annotation (@Targetproperty) or we standardize on an "identifier" property.

IdentifierConfigContext context = new SequenceIdentifierConfigContext([namespace: "purchaseOrder", targetProperty: "orderNumber"])
purchaseOrder.orderNumber = identifierService.generateSequenceIdentifier(context)

But anyway, exposing an API through an expression language would allow us to use the same API throughout the application, not just in runtime config i.e. party type, product type.

We'd probably want to use an existing expression language (like Spring EL similar to how Spring Security handles authorization rules)
https://docs.spring.io/spring-security/reference/5.8/servlet/authorization/expression-based.html

or template library like Thymeleaf.

interesting. We could definitely look at something like that to make the config simpler, but I think regardless we should consider this an admin feature that needs public documentation. Do you need custom ids? Here's how to define that for your implementation.

I think the generate method probably needs two arguments:

• a context that can be filled with config properties and other metadata used by the ID generator
• the object for which the identifier is being generated

The Context could be specific to ID generation or perhaps we have some generic context object that can be used to store values needed for the work done by the specific implementation. For example, this would be very similar to a context object that gets passed to a template rendering component. I haven't looked at IdentifierGeneratorParams yet (or had time to think this through), but it seems this class is tighter version of the Context, so I think this works for now.

Ah, you pass the object only when you you need it. So I think that's fine for now. Perhaps we could find a way to pass both the "object" and "configuration" in a more magical / implicit way i.e. if you call generate with no args, the abstract class creates a IdentifierGeneratorParams and populates it with the config properties and anything else that might be available.

I actually originally had the generate method passing in the entity as a requirement but it got complicated because there were some cases where two different entities were sharing the same format for generating ids so it got wonky with all the typing.

I was struggling with the balance of "this should be driven 100% via properties" and "well we need to provide certain params in certain situations directly in the code". With the custom params it's kinda inevitable that we need to provide them via code, but for everything else it can be determined purely via properties.

I suppose I was trying too hard to make the identifiers not need to rely on a specific entity (by making it optional), but given that the property config is entity specific, maybe we should just lean in and say an identifier service implementation is for a specific entity, full stop. Any entity that uses an identifier must define their own service.

It would be a really good idea to take some time now to think about how all of the different generate() methods work, document their requirements / behaviors, and come up with a design / unifying interface that will allow us to refactor these one-by-one in the future.

• PO identifier requires a sequential identifier that is being stored on the organization
• Product identifiers use a random number identifier unless there's a different format provided by the product type (i don't think we don't currently support sequential number identifiers here, but we should)
• Shipment identifiers are assigned (should be deprecated in favor of separate)
• Requisition identifiers are random number identifiers
• yada yada yada

Potential Tech Huddle conversation to be had ... do we provide the config properties as part of the Params / Context object passed to the generate method OR do we always inject these into the service using the Value annotation.

yeah we should discuss. I think @value is the recommended approach in 99% of cases. There are situations here where we need to fetch individual settings by feature type, and for those it makes more sense to use the config service since we don't want to assume which feature settings are defined since they'll vary between implementations.

also the only reason it's using the old way here is because I didn't want to change the code until I had time to revisit and properly refactor it.

Seeing this implementation makes me wonder if the appropriate method name for this is ...

Integer countByIdentifier(String identifier);

That way it's more precise and we're not implying anything by it's invocation. However, maybe the implication ("I want to know how many duplicates are there") is the point.

yeah I wanted to be specific about what we need, which is "how many copies of this id already exist in the database". Though I suppose countByIdentifier implies something similar

This does not have to happen now, but passing the organization object here (either in params or as an argument on its own) seems like the right approach.

yeah I agree. Just didn't want to do the refactor at this point since it was already getting unwieldy.

Seeing customKeys being used for the first time, I'm wondering if there's a more intutive name for that. customKeys implies that we need to pass in a list of map keys.

You can leave it for now, but something as simple as data, customData, or customProperties might suffice.

I like customProperties